Skip to content

joy_teleop: Do not require deadman input for axis mapping#71

Open
russkel wants to merge 3 commits intoros-teleop:foxy-develfrom
russkel:no-deadman
Open

joy_teleop: Do not require deadman input for axis mapping#71
russkel wants to merge 3 commits intoros-teleop:foxy-develfrom
russkel:no-deadman

Conversation

@russkel
Copy link
Contributor

@russkel russkel commented Jul 18, 2021

Hello,

While attempting to use this teleop package for my purposes, I found I couldn't simply map an axis without specifying a "deadman switch" which in this case is more accurately a trigger. These changes improve the behaviour of the mapping.

  • Scale and Offset aren't really required parameters because defaults of 1.0 and 0 are used respectively. Removes tests and checks.
  • Check that all deadman inputs are actually satisfied.
  • Remove requirement for deadman input to map an axis. Publishes on relevant axis/button changes since last iteration.
  • Slight clean ups and refactoring.

This code should work on foxy, but has been tested on galactic only.

Russ

@russkel
Copy link
Contributor Author

russkel commented Aug 28, 2021

@bmagyar could this get a review please?

self.assertEqual(joy_teleop_process.exit_code, 1)
self.assertTrue('must have an axis, button, or value' in joy_teleop_process.output)

def test_teleop_axis_mappings_missing_offset(self):
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 these tests still hold value in showing that things are handled despite some things missing. Is it ok to leave them?

Copy link
Contributor Author

@russkel russkel Sep 4, 2021

Choose a reason for hiding this comment

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

These tests are expecting an error when there isn't one after the changes in this PR.

Do you mean you also want the checks for empty offset and scale put back into the code?

@bmagyar
Copy link
Member

bmagyar commented Sep 3, 2021

Please specify what your target distro is with this

@russkel
Copy link
Contributor Author

russkel commented Sep 4, 2021

Please specify what your target distro is with this

I use galactic, personally. I don't see why this wouldn't work on foxy as well.

@russkel
Copy link
Contributor Author

russkel commented Oct 5, 2021

Hi @bmagyar - poke.

@AdronTech
Copy link

Any updates on this PR?

@russkel
Copy link
Contributor Author

russkel commented Mar 27, 2022

Seems to fail all CI tests now. I will fix this when I get a moment and hopefully it can be merged.

@blaine141
Copy link

I am still interested in this change

Copy link

@blaine141 blaine141 left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@blaine141
Copy link

Appears to be resolving #74

@russkel
Copy link
Contributor Author

russkel commented Mar 12, 2023

Yeah I would have liked to have had these changes merged years ago.

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.

4 participants