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

[SUN-887] Initial joint values are read when the hardware interface starts #2

Merged
merged 10 commits into from
Nov 21, 2024

Conversation

tgaspar
Copy link

@tgaspar tgaspar commented Nov 19, 2024

Description

This PR adds code that reads joint state values from RWS before the hardware interface is started. It is important to note that the code introduced here does not make use of the existing RWS managers that would be available vie abb_librws because those are incompatible with RWS2.0. For the sake of not complicating things, I added a simple method that makes the HTTP GET request to the correct address to retrieve the joint values.

Some reading material regarding the issue:

Test plan

The test plan revolves mainly around making sure everything builds. The functionality will be tested by me on the real robot. Some additional changes might come into place after the test are done. However, the bulk of the work is here.

Checklist for Reviewers

Reviewer 1: @fgrcar
Reviewer 2: @JenniferBuehler

Before a PR to this repo can get approved and merged, the following needs to be checked by both reviewers:

  • The test plan was executed and verified by:
    • Reviewer 1
    • Reviewer 2
  • Documentation added, function/classes headers clean?
    • Reviewer 1 check
    • Reviewer 2 check
  • Has the README.md been updated, or not required?
    • Reviewer 1 check
    • Reviewer 2 check
  • Has the CHANGELOG been updated?
    • Reviewer 1 check
    • Reviewer 2 check
  • Have unit tests been added where needed?
    • Reviewer 1 check
    • Reviewer 2 check
  • Any testing not covered by unit tests was done manually by the reviewers.
    • Reviewer 1
    • Reviewer 2
  • Version ticks in package.xml, CMakeLists.txt and/or setup.py were made.
    • Reviewer 1 check
    • Reviewer 2 check
  • Licensing updates were added, or not required.
    • Reviewer 1 check
    • Reviewer 2 check
  • New dependencies added to requirements.txt, setup.py and package.xml - or: not applicable.
    • Reviewer 1 check
    • Reviewer 2 check

Copy link

@JenniferBuehler JenniferBuehler left a comment

Choose a reason for hiding this comment

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

I built it, it works. Nice!

I have general concerns about how we keep this in sync with upstream (or is our fork so far evolved?) but that's a separate question to this PR. I asked on slack already :)

README.md Outdated Show resolved Hide resolved
@JenniferBuehler
Copy link

No idea why I can't tick the checkmarks on the PR description... maybe it's the recent apt upgrade result that I was semi-forced to do and has already given me a bit of headache (with nvidia, whatta surprise)

Copy link

@fgrcar fgrcar left a comment

Choose a reason for hiding this comment

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

I can imagine how painful this was 🫂.
Check the comments and you're good to go.

README.md Outdated Show resolved Hide resolved
abb_hardware_interface/src/utilities.cpp Show resolved Hide resolved
abb_hardware_interface/src/utilities.cpp Outdated Show resolved Hide resolved
abb_hardware_interface/src/utilities.cpp Show resolved Hide resolved
abb_hardware_interface/src/utilities.cpp Outdated Show resolved Hide resolved
@fgrcar
Copy link

fgrcar commented Nov 21, 2024

Btw, I can't tick the checkboxes in the description, so something is wrong with the permissions I guess.

@tgaspar tgaspar merged commit bb63544 into rolling Nov 21, 2024
@tgaspar tgaspar deleted the SUN-887-initial-joint-values branch November 21, 2024 15:37
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