Skip to content

surpress warnings for known unused event types#307

Open
ethanholter wants to merge 2 commits intoros-drivers:ros2from
ethanholter:ros2
Open

surpress warnings for known unused event types#307
ethanholter wants to merge 2 commits intoros-drivers:ros2from
ethanholter:ros2

Conversation

@ethanholter
Copy link
Copy Markdown

When using newer gamepads such as the Dualshock 5 it's pretty common to see messages along the lines of Unknown event type 1623. These messages can be useful for maintainers of the joystick_drivers package but the average joe doesn't really care.

In some scenarios, such as with the aforementioned Dualshock 5, these messages will be printed in a torrential flood at whatever rate the joy_node is publishing at. It is quite frustrating to suddenly need to sift through hundreds of lines of repeated log messages simply because I accidentally bumped my controllers touch pad.

This pull request proposes adding a list of SDL events which are explicitly known to pose no function within the joy_node. This way any actually anomalous unknown events can still be logged without the worry of drowning in useless log messages

Copy link
Copy Markdown
Contributor

@peci1 peci1 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.

I've suggested a few changes, so please have a look.

Otherwise, there's a similar piece of code in game_controller.cpp (

switch (e.type) {
case SDL_CONTROLLERAXISMOTION: {
should_publish = handleControllerAxis(e.caxis);
break;
}
case SDL_CONTROLLERBUTTONDOWN: {
should_publish = handleControllerButtonDown(e.cbutton);
break;
}
case SDL_CONTROLLERBUTTONUP: {
should_publish = handleControllerButtonUp(e.cbutton);
break;
}
case SDL_CONTROLLERDEVICEADDED: {
handleControllerDeviceAdded(e.cdevice);
break;
}
case SDL_CONTROLLERDEVICEREMOVED: {
handleControllerDeviceRemoved(e.cdevice);
break;
}
case SDL_JOYAXISMOTION: // Ignore joystick events, they are duplicates of CONTROLLERDEVICE.
case SDL_JOYBALLMOTION:
case SDL_JOYHATMOTION:
case SDL_JOYBUTTONDOWN:
case SDL_JOYBUTTONUP:
case SDL_JOYDEVICEADDED:
case SDL_JOYDEVICEREMOVED: {
break;
}
default: {
RCLCPP_INFO(get_logger(), "Unknown event type %d", e.type);
break;
}
}
). For consistency, please also add similar behavior there (if you're unsure about which events to ignore there, just change the RCLCPP_INFO into _ONCE).

if (success == 1) {
// Succeeded getting an event
if (e.type == SDL_JOYAXISMOTION) {
if (ignored_sdl_events.count(e.type) == 1) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd move this check to the end of the ifs (both logically and performance-wise).

If you'd want to turn this if-else spaghetti into a switch statement, it would be great (but not needed for this PR to get merged).

} else if (e.type == SDL_JOYDEVICEREMOVED) {
handleJoyDeviceRemoved(e);
} else {
RCLCPP_INFO(get_logger(), "Unknown event type %d", e.type);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
RCLCPP_INFO_ONCE(get_logger(), "Unknown event type %d", e.type);

This will handle the rest of the events that could pop up in the future or with different hardware. The only caveat is that it will only log the first unknown event type, but I'd say that's okay (normal users wouldn't care, developers could temporarily disable the _ONCE check if working on something related to new events).

#include <memory>
#include <stdexcept>
#include <string>
#include <thread>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
#include <thread>
#include <unordered_set>

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.

2 participants