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

Event Device Support (evdev) #1943

Merged
merged 26 commits into from
Feb 20, 2024

Conversation

votti
Copy link

@votti votti commented Dec 30, 2022

Background

As described in #1942, a plugin to support to setup listeners to event devices such as USB controller, joysticks or keyboards would be useful.

Implementation

This implementation builds on the EvDevKeyListener and is based on the bluetooth_audio_buttons plugin.

The following details differ from the bluetooth_audio_buttons plugin:

  • initialize the listeners from the configuration. I have tried to document the format in the docstring. Is there a better place to do so?
  • rpc command is bound with deference=False. Otherwise plugins, eg next_song, may not be initialized yet upon initialization. Is there a better way?

@s-martin s-martin added enhancement future3 Relates to future3 development labels Dec 30, 2022
@s-martin s-martin linked an issue Dec 30, 2022 that may be closed by this pull request
@s-martin
Copy link
Collaborator

@pabera @ChisSoc

@ghost
Copy link

ghost commented Dec 30, 2022

Nice addition!

Regarding the documentation: The best way to go would be to write a Ueer Guide Page. The live in docs/sphinx/userguide. Just create a new .rst file and add to in the index.rst. See the bluetooth doc as an example.

The user guide needs

  • How to activate this plugin
  • How to find out the device_name that is needed for the configuration
  • How to find out the button id
  • And a complete configuration example, please

Ah, yes, dereference. If I remember correctly this has to do with the order of the loading of the plugins. Try loading your plugin last.

@votti
Copy link
Author

votti commented Dec 30, 2022 via email

@ghost
Copy link

ghost commented Jan 1, 2023

Ah yes, finalize it was.

Don't worry about the performance impact. It is negligible.

@votti
Copy link
Author

votti commented Jan 2, 2023

I added now a documentation.
It would be great if someone could test it. As a device eg an USB mouse works well.

Copy link
Collaborator

@pabera pabera left a comment

Choose a reason for hiding this comment

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

I read through your documentation. I found a few typos which I corrected myself rather than calling them out. I committed them to the same branch. I'd like @ChisSoc to do a last check on the Python code.

Great addition. (all though I wonder how my 3 year old would use a USB mouse to rule their Phoniebox 🤔)

src/jukebox/components/controls/event_devices/README.rst Outdated Show resolved Hide resolved
src/jukebox/components/controls/event_devices/__init__.py Outdated Show resolved Hide resolved
@votti
Copy link
Author

votti commented Jan 7, 2023

Thanks for the comments, I will address them!

"(all though I wonder how my 3 year old would use a USB mouse to rule their Phoniebox thinking)"

The USB mouse was more a very general illustration that this works with any USB device, not necessarily an endorsement :)
The beter usecase for this is to use a cheap Joystick controller board in order to hook up arcade buttons in via USB in case the GPIO pins are occupied otherwise.
I am using: https://de.aliexpress.com/item/1005001604578101.html?spm=a2g0o.order_list.order_list_main.5.26875c5fq1PFcA&gatewayAdapt=glo2deu

@votti
Copy link
Author

votti commented Jan 10, 2023

It seems that flake8 is failing due to flake8 violations in code not touched by this PR.
Schould I do a separare PR fixing all the pre-existing violations or rather fold the fixes into this PR?

@pabera
Copy link
Collaborator

pabera commented Apr 7, 2023

Schould I do a separare PR fixing all the pre-existing violations or rather fold the fixes into this PR?

A separate PR would be ok.

Would you still be interested in merging this?

@AlvinSchiller
Copy link
Collaborator

It seems that flake8 is failing due to flake8 violations in code not touched by this PR. Schould I do a separare PR fixing all the pre-existing violations or rather fold the fixes into this PR?

For the fixes see #1989

@votti
Copy link
Author

votti commented Aug 13, 2023

Hi @pabera ,

Sorry for being unresponsive, I had quite lot of personal events that diminished my free time.

I rebased the PR to the latest dev branch, so the flake8 should be fixed and the cosmetic issues should be addressed.

@pabera
Copy link
Collaborator

pabera commented Aug 14, 2023

I have fixed the flake8 issue in another PR (#2057). If you pull all changes again, you should be able to pass flake8 properly

votti and others added 6 commits August 15, 2023 22:33
This implements an "event device" listener plugin,
that enables the user to configure the phoniebox to
respond to events from an "event device" (device under /dev/input).
This incluces eg button presses from an USB controller or keyboard.
Includes a detailed how-to as well as example config
This is an actual usecase in case someone wants to setup a device
and figure out the button ids by looking at the logs
- Remove duplicated section as suggested
- Add Zero Delay Arcade USB Encoder usecase
@votti
Copy link
Author

votti commented Aug 15, 2023

Hi there,
I rebased again and run flake8 locally - now all looks clean on my side.

@votti
Copy link
Author

votti commented Aug 15, 2023

I also found by chance that in the old Jukebox there was already an implementation/reference about this: https://github.com/MiczFlor/RPi-Jukebox-RFID/blob/develop/components/controls/buttons_usb_encoder/README.md?plain=1
Should I link this in the readme?

@s-martin
Copy link
Collaborator

s-martin commented Dec 4, 2023

@votti would you mind rebasing latest changes from future3/develop? Biggest change is probably that docs are now markdown.

I think then we could merge this PR

@votti
Copy link
Author

votti commented Jan 17, 2024

@pabera I am away for holidays so no access to a pi. However I just managed to get the docker dev environment (including evdev devices) runing, so I may be able to work on it till the end of the week.

No hard feelings if this does not make the next release. I think having the configuration file in a final form before merging it is really important.

pre-commit fix Vito Zanotelli added 2 commits January 18, 2024 22:34
This is tighly modeled after gpio.
In contrast to GPIO, this is a list of devices containing
each one or more input/output devices.
Eg a Joystick has keys but potentially also rumble or leds.

Currently only very simply input devices (buttons) are supported.
This structer should enable to extend this easily.
As suggested this should be a separate file, similar to the gpioz config
@pabera pabera modified the milestones: v3.5, v3.6 Jan 18, 2024
pre-commit fix Vito Zanotelli added 2 commits January 18, 2024 23:53
Now the config for evdev is akin to GPIO.
The big difference is that multiple devices with each
multiple input/output devices are supported.

Note that the backend is still the old event device listener backend.
Thus to support more features, the backend would need to be
quite drastically overhauled.
@votti
Copy link
Author

votti commented Jan 18, 2024

I now aligned the configuration structure with gpio.yaml.
In contrast to gpio, multiple devices each with their own input/output_devices are supported.
Note that currently only input_devices the Button type with the on_press action is implemented.

While extending the config to such new functionalities is now easy, this code is still relying on the pre-existing EventDeviceListener code backend. Expanding this mechanisms to support more features (eg long press etc) will require significant changes on this backend.

This functionality has been tested now in the Docker environment as I currently dont have access to my raspi. However due to the generic nature of the matter, I would expect no issues.

" Investigate why I indicated that the evdev module should be listed last." - I did not find a good reason after trying also different orders. Maybe I just meant that while it looks in the provided example that it should be the first entry, it actually just should be appended.

pre-commit fix Vito Zanotelli added 8 commits January 19, 2024 00:33
The attribute 'device_request' of EvDevKeyListener is now called 'device_name_request'
Fails if there is no device_name and setts proper default for 'exact'
This is required for some tests.

Following the instructions from issue MiczFlor#2050
Specifically: MiczFlor#2050 (comment)
This reverts commit 75161b5.

As this was not working
Currently it is hard to install zmq (MiczFlor#2050) and for
CI pytest `zmq` is not available.
As the test do not really require `jukebox.publishing`
running, mocking it in the test avoids it being imported.

Thus the tests do not require `zmq` to be installed.
@votti
Copy link
Author

votti commented Jan 19, 2024

I now also added some tests and fixed some bugs/edge cases found during testing.

One issue I encountered is that zmq is not installed for the Github CI testing.
First I tried to fix the Github CI but then decided to better just use mocks to make my tests independent of zmq.

I will add an issue that it would be nice to eventually have zmq also in our github action workflow used for testing (done: #2219 )

@@ -0,0 +1,59 @@
devices: # A list of evdev devices each containing one or multiple input/output devices
joystick: # A nickname for a device
device_name: DragonRise Inc. Generic USB # Device name
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this just a pretty name or actually relevant to access/call the device?

Copy link
Author

Choose a reason for hiding this comment

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

The 'device_name' is relevant as it is used to get the device number (in contrast to the nickname line 2 which is cosmetic).

The docs contain a section how to look it up: https://github.com/MiczFlor/RPi-Jukebox-RFID/blob/49839adb01019cb296d67dc29c06cd9a6af0bc62/documentation/builders/event-devices.md#identifying-the-device_name

@@ -0,0 +1,185 @@
""" Tests for the evdev __init__ module
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like the fact that you have put effort into tests as well. It keeps pushing the bar. It's something we haven't pushed for in the past but indeed allows to be more confident when others make changes in the future.

@pabera pabera mentioned this pull request Feb 5, 2024
2 tasks
@AlvinSchiller AlvinSchiller mentioned this pull request Feb 14, 2024
@s-martin
Copy link
Collaborator

Could be merged?

@votti
Copy link
Author

votti commented Feb 19, 2024

Let me know if there is anything still required from my side!
If not I would appreciate if this would be merged before it diverges again from the main develop/future3 branch.

Copy link
Collaborator

@pabera pabera left a comment

Choose a reason for hiding this comment

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

Approved.

Last question @votti .. How do you execute the tests in this scenario?

@votti
Copy link
Author

votti commented Feb 20, 2024

The tests are executed as part of the standard "pytest" call.
eg: https://github.com/MiczFlor/RPi-Jukebox-RFID/actions/runs/7951763478/job/21705563833?pr=1943#step:5:30
in the ci:

test/evdev/test_evdev_init.py::TestInputDevicesToKeyMapping::test_mapping_with_missing_key_code PASSED [ 24%]
test/evdev/test_evdev_init.py::TestInputDevicesToKeyMapping::test_mapping_with_missing_type PASSED [ 27%]
test/evdev/test_evdev_init.py::TestInputDevicesToKeyMapping::test_mapping_with_supported_input_type_and_key_code PASSED [ 29%]
test/evdev/test_evdev_init.py::TestInputDevicesToKeyMapping::test_mapping_with_unsupported_action PASSED [ 32%]
test/evdev/test_evdev_init.py::TestInputDevicesToKeyMapping::test_mapping_with_unsupported_input_type PASSED [ 35%]
test/evdev/test_evdev_init.py::TestParseDeviceConfig::test_parse_device_config PASSED [ 37%]
test/evdev/test_evdev_init.py::TestParseDeviceConfig::test_parse_device_config_missing_device_name PASSED [ 40%]
test/evdev/test_evdev_init.py::TestParseDeviceConfig::test_parse_device_config_missing_exact PASSED [ 43%]
test/evdev/test_evdev_init.py::

@s-martin s-martin merged commit 07208e6 into MiczFlor:future3/develop Feb 20, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement future3 Relates to future3 development
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🚀 | Add Event Device plugin to support USB controllers
4 participants