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 RTDEClient constructor with vector recipes #143

Merged
merged 5 commits into from
Aug 29, 2023

Conversation

dagare
Copy link
Contributor

@dagare dagare commented Mar 14, 2023

No description provided.

@codecov
Copy link

codecov bot commented Mar 20, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.14% 🎉

Comparison is base (f328157) 70.21% compared to head (45965ce) 70.35%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #143      +/-   ##
==========================================
+ Coverage   70.21%   70.35%   +0.14%     
==========================================
  Files          69       69              
  Lines        2535     2547      +12     
  Branches      324      324              
==========================================
+ Hits         1780     1792      +12     
  Misses        568      568              
  Partials      187      187              
Files Changed Coverage Δ
include/ur_client_library/rtde/rtde_client.h 100.00% <ø> (ø)
src/rtde/rtde_client.cpp 55.80% <100.00%> (+1.77%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@urrsk
Copy link
Member

urrsk commented Mar 20, 2023

@dagare Thanks for this good suggestion.
It would be great If you could add some test to cover and verify this in the PR.
A suggestion could be to extend this test file: https://github.com/UniversalRobots/Universal_Robots_Client_Library/blob/master/tests/test_rtde_client.cpp

@dagare
Copy link
Contributor Author

dagare commented Mar 21, 2023

I will make a new addition to this PR

Copy link
Collaborator

@fmauch fmauch left a comment

Choose a reason for hiding this comment

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

@dagare Thank you for the contribution. Could you please provide some insight why you see this as beneficial?

I'm considering to suggest only allow passing recipes to the RTDEClient class as vectors directly, so parsing a file (if any) would have to be done a level higher up.

@RobertWilbrandt what do you think?

@fmauch fmauch added this to the Release 1.3.3 milestone Jul 14, 2023
@RobertWilbrandt
Copy link
Collaborator

Agreed, i think moving the file handling out of RTDEClient would be a good step.

@urrsk
Copy link
Member

urrsk commented Aug 1, 2023

I will suggest we add the support first.
When we can consider to deprecate the file dependent constructor later

Copy link
Collaborator

@urmahp urmahp 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 @dagare for the contribution.

I have added a small adjustment, but other than that it looks good.

src/rtde/rtde_client.cpp Outdated Show resolved Hide resolved
@urmahp
Copy link
Collaborator

urmahp commented Aug 23, 2023

Before merging I suggest that we add checks in the constructor to see if the recipes are empty and whether "timestamp" is part of the output_recipe.

@urmahp
Copy link
Collaborator

urmahp commented Aug 29, 2023

Thank you for the contribution @dagare.

@urmahp urmahp merged commit e85b3ae into UniversalRobots:master Aug 29, 2023
18 of 19 checks passed
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.

None yet

5 participants