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

Added commands, version check and now throws exception #1

Open
wants to merge 3 commits into
base: pr
Choose a base branch
from

Conversation

urmarp
Copy link

@urmarp urmarp commented Nov 18, 2022

Added tests for dashboard client

urrsk and others added 3 commits October 29, 2022 10:42
The dockerfile is no longer needed
as the repo now uses the docker image
@codecov
Copy link

codecov bot commented Nov 21, 2022

Codecov Report

Base: 74.23% // Head: 73.15% // Decreases project coverage by -1.08% ⚠️

Coverage data is based on head (ecd7747) compared to base (a50f530).
Patch coverage: 50.53% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##               pr       #1      +/-   ##
==========================================
- Coverage   74.23%   73.15%   -1.09%     
==========================================
  Files          89       90       +1     
  Lines        3198     3382     +184     
  Branches      343      378      +35     
==========================================
+ Hits         2374     2474     +100     
- Misses        612      677      +65     
- Partials      212      231      +19     
Impacted Files Coverage Δ
include/ur_client_library/ur/dashboard_client.h 100.00% <ø> (ø)
src/ur/dashboard_client.cpp 54.31% <41.42%> (-12.36%) ⬇️
tests/test_dashboard_client.cpp 77.77% <77.77%> (ø)
examples/dashboard_example.cpp 23.33% <100.00%> (+2.64%) ⬆️
tests/test_tcp_server.cpp 99.23% <0.00%> (+0.75%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Owner

@urrsk urrsk left a comment

Choose a reason for hiding this comment

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

Great job.
Please look at my comments


using namespace urcl;

// In a real-world example it would be better to get those values from command line parameters / a
// better configuration system such as Boost.Program_options
const std::string DEFAULT_ROBOT_IP = "127.0.0.1";
const std::string DEFAULT_ROBOT_IP = "10.53.253.22";
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
const std::string DEFAULT_ROBOT_IP = "10.53.253.22";
const std::string DEFAULT_ROBOT_IP = "127.0.0.1";

Copy link
Owner

Choose a reason for hiding this comment

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

Suggest reverting this line

@@ -64,6 +65,8 @@ int main(int argc, char* argv[])
return 1;
}

my_dashboard->commandCloseSafetyPopup();
Copy link
Owner

Choose a reason for hiding this comment

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

Please add a comment on why this is a good idea.
Remember this is an example to get people started

bool commandRunning();

/*!
* \brief Send Is program saved command
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
* \brief Send Is program saved command
* \brief Send "Is program saved" request command

/*!
* \brief Send Is program saved command
*
* \return True succeeded
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
* \return True succeeded
* \return True if the program saved

bool commandIsProgramSaved();

/*!
* \brief Send Is in remote control command
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
* \brief Send Is in remote control command
* \brief Send "Is in remote control" request command

/*!
* \brief Send Is in remote control command
*
* \return True succeeded
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
* \return True succeeded
* \return True if it is in remote control

URCL_LOG_WARN("Did not got the expected \"%s\" respone within the timeout. Last respone was: \"%s\"",
expected.c_str(), response.c_str());
URCL_LOG_WARN("Did not got the expected \"%s\" response within the timeout. Last response was: \"%s\"",
expected.c_str(), response.c_str()); // Is a warning here so retryCommand does not throw when retrying
Copy link
Owner

Choose a reason for hiding this comment

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

Change it to an exception and make "retryCommand()" catch the exception

bool ret = std::lexicographical_compare(parsedRef, parsedRef + 4, parsedC, parsedC + 4);
if (!ret)
{
throw UrException("Polyscope software version required is: " + ref + ", but actual version is: " + c);
Copy link
Owner

Choose a reason for hiding this comment

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

Will suggest that we throw the exception in the "command" function to make it obvious witch one is the one that is not supported by the robot or pass the command name to this function

bool DashboardClient::commandPowerOff()
{
if (!lessThanVersion("5.0.0", "3.0", polyscope_version_))
return false;
Copy link
Owner

Choose a reason for hiding this comment

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

Will it ever return false?


using namespace urcl;

// std::string ROBOT_IP = "192.168.56.101";
Copy link
Owner

Choose a reason for hiding this comment

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

Suggest that we remove this line

@fmauch
Copy link

fmauch commented Nov 22, 2022

I've merged the changes from this PR into UniversalRobots#127 and will do the review / final changes there.

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