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

Hid joystick interface #4226

Open
wants to merge 3 commits into
base: master
from

Conversation

@a-chol
Copy link
Contributor

commented Oct 24, 2018

This implements a joystick feature, including a joystick_task function called from tmk, specific keycodes for joystick buttons and a usb hid interface for v-usb and lufa.
I tested the v-usb backend, as I have a compatible board, but only provide a compilable-but-untested implementation for lufa. Runtime tests of this backend is appreciated.
In order to test, you have to add JOYSTICK_ENABLE = yes to your rules.mk and

#define JOYSTICK_BUTTON_COUNT 8
#define JOYSTICK_AXES_COUNT 2

in your config.h

Any feedback is appreciated.

docs/feature_joystick.md Outdated Show resolved Hide resolved
quantum/joystick.h Outdated Show resolved Hide resolved
JS_BUTTON29,
JS_BUTTON30,
JS_BUTTON31,
JS_BUTTON_MAX = JS_BUTTON31,

This comment has been minimized.

Copy link
@mechmerlin

mechmerlin Oct 24, 2018

Contributor

Maybe add a

Suggested change
JS_BUTTON_MAX = JS_BUTTON31,
JS_BUTTON_MIN = JS_BUTTON0

This comment has been minimized.

Copy link
@jackhumbert

jackhumbert Oct 24, 2018

Member

@mechmerlin it looks like your suggestion replaces that line - I think you can make one that adds a new line as well, right?

This comment has been minimized.

Copy link
@drashna

drashna Oct 29, 2018

Member

Honestly, I think that this is fine. the "MAX" line will report the number of entries (since the enum starts at 0). So for any code that needs to check this stuff, JS_BUTTON_MAX could be used to check against.

return;
}

joystick_report_t r = {

This comment has been minimized.

Copy link
@mechmerlin

mechmerlin Oct 24, 2018

Contributor

This code is very similar to the one in vusb.c, anyway to avoid some of the code repetition?

This comment has been minimized.

Copy link
@a-chol

a-chol Oct 25, 2018

Author Contributor

There might be a factorisation opportunity, I think looking at the implementation for other platforms will be informative. I am also still not sure of dependencies between libraries, so I'll give it a bit more thoughts.

@drashna

This comment has been minimized.

Copy link
Member

commented Oct 24, 2018

It looks like this is added to VUSB and LUFA but not chibios, or arm_atsam. Is there a reason why, other than a lack of hardware?

@drashna drashna added the enhancement label Oct 24, 2018

@a-chol

This comment has been minimized.

Copy link
Contributor Author

commented Oct 25, 2018

I'm AFK for a few days so I'll apply your suggestions when I come back.
I only implemented VUSB and Lufa mostly for hardware reasons. Giving Lufa a shot was a way to bring in more feedback and there is no reason to spare the others.

@a-chol

This comment has been minimized.

Copy link
Contributor Author

commented Oct 25, 2018

By the way, if anyone else has the hardware and feels like giving it a try, please.do. I'm not sure of the process of collaborating on a pull request on github though, maybe submitting a PR to my fork?

@drashna

This comment has been minimized.

Copy link
Member

commented Oct 25, 2018

Also, don't forget that if you're adding a feature page, you'll need to link it in /docs/_summary.md and /docs/_sidebar.md

@a-chol a-chol referenced this pull request Nov 3, 2018
In this example, the first axis will be read from the D7 pin, using an analogRead, whereas the second axis will not be read.
In order to give a value to the second axis, you can do so in any custimizable entry point of quantum : as an action, in process_record_user or in matrix_scan_user, or even in joystick_task(void) which is call even when no key has been pressed.
You assign a value by writing to joystick_status.axes[axis_index] a signed 8bit value (-127 to 127). Then it is necessary to assign the flag JS_UPDATED to joystick_status.status in order for the change to be taken into account.

This comment has been minimized.

Copy link
@coredump

coredump Nov 28, 2018

Would like to have some clarification here. Is the joystick_config_t array necessary if you only want to use virtual joystick axis like described below, and if it is necessary, how to declare it since there is no actual pin with analog data?

This comment has been minimized.

Copy link
@a-chol

a-chol Nov 28, 2018

Author Contributor

Yes, the joystick_config_t is still necessary. I might be able to make it optional if all axes are virtual though. For now, the config would be as described in the doc, with all pin definitions defined as JS_VIRTUAL_AXIS. The two values after define the range of analog value read to map to -127;127, so there is no use for it for virtual axes.

This comment has been minimized.

Copy link
@a-chol

a-chol Nov 30, 2018

Author Contributor

@coredump I saw your post on reddit and that you are using a pro micro. Have you tried building a firmware with the joystick feature enabled yet?

This comment has been minimized.

Copy link
@coredump

coredump Dec 1, 2018

I did, it compiled, but I didn't add axis, just buttons. I got an error on Windows, like USB device not recognized. I saw some people on the Lufa github saying something about avoiding declaring a serial for the device (there's a env variable for that), but I haven't tested it yet.

This comment has been minimized.

Copy link
@coredump

coredump Dec 4, 2018

So, for some reason this is not compiling anymore. It's complaining about not finding the joystick_config_t or anything defined on the quantum/* files related to the joystick support. I tried to find out why, but afaik the rules.mk is correct, and the common_features.mk should be adding the files to the compilation, but it's not happening.
There's a gist here with the error: https://gist.github.com/coredump/83b08aaab5e194f74785c2b94dd8c38d

(also I am running from your branch, @a-chol )

This comment has been minimized.

Copy link
@a-chol

a-chol Dec 6, 2018

Author Contributor

Good new for linux! Can you provide me the diff of the changes you made, or offer a PR on my fork (if that's a thing, I'm not well versed in the usages of github).
I use USBlyzer on windows, did you use that or does it give any more info. I don't really see how the report descriptor could be misaligned.
Maybe you could try returning early from send_joystick_packet in lufa.c to avoid any report sending ; to check that it's not the report that is creating the error.
You could also vary the number of buttons : I suspect that using a number of buttons that is not a multiple of 8 wouldn't be handled well. I think I send the whole eight-bit array when I should only be sending the relevant bits. I'll look into it tomorrow.

This comment has been minimized.

Copy link
@coredump

coredump Dec 7, 2018

I forgot to let you know that I added a PR.

This comment has been minimized.

Copy link
@a-chol

a-chol Dec 7, 2018

Author Contributor

Excellent, it's now merged into this one! I'll add your fix for byte-aligned reports to the other existing implementations. Thanks for your help, hope you enjoy the feature!

This comment has been minimized.

Copy link
@drashna

drashna Feb 5, 2019

Member

This got resolved, and added in, correct?

This comment has been minimized.

Copy link
@a-chol

a-chol Feb 5, 2019

Author Contributor

Yes, this got merged in my branch.

@a-chol a-chol force-pushed the a-chol:hid_joystick branch from 0072ac2 to 533ca73 Nov 29, 2018

@a-chol

This comment has been minimized.

Copy link
Contributor Author

commented Nov 30, 2018

I just pushed an untested implementation of the hid joystick interface for chibios. It builds with the chibios_test keyboard, but lacks the analog read function. I couldn't find a simple function like the analogRead from the drivers/avr folder. It can still be usefull as is though.
So far, there is an implementation for AVR v-usb (tested), AVR LUFA (untested) and ARM chibios (untested).
If anyone can give this a try on any of those untested platforms, it would be of great help.

@drashna

This comment has been minimized.

Copy link
Member

commented Dec 2, 2018

What about ATSAM? (massdrop boards)

@a-chol

This comment has been minimized.

Copy link
Contributor Author

commented Dec 2, 2018

I can give it a try but without the hardware to test, it's really hit-or-miss. I'm afraid that pushing incomplete implementation in this PR requires it to be validated before merging, which in the end postpones the merge for other protocols.
Unless we manage to have a tester for each protocol, I don't see that PR being merged anytime soon.

@drashna

This comment has been minimized.

Copy link
Member

commented Dec 6, 2018

Also, it looks like you have a merge conflict here.

@a-chol a-chol force-pushed the a-chol:hid_joystick branch from 0e8c2e4 to 384dddb Dec 6, 2018

@drashna

This comment has been minimized.

Copy link
Member

commented Jan 25, 2019

It looks like there are some merge conflicts that need to be resolved

@a-chol a-chol force-pushed the a-chol:hid_joystick branch from 3dc0c64 to 030f86c Jan 30, 2019

@a-chol

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2019

The merge conflicts are resolved.
The feature is reaching completion. I've validated the adc reading code with my own keyboard, properly reading and mapping the inputs to the int8 value range.
Axis configuration now supports defining the input, output and ground pins to configure. When configured, the pin configuration is saved, then restored so that the qmk matrix scan, relying on input pins not changing, works properly.
Axis configuration also support defining calibration values using a low, resting and high raw adc reference values. Sometimes the resting position doesn't return the middle value of the extreme positions, but it is essential that the resting position return a value of 0 once mapped to the hid range, so a piecewise transfer function between raw adc values and the hid output range was required.
Beside these changes, I implemented the reading code in a function called from a weak linkage function, so that any keyboard can redefine the adc reading procedure and still call the original quantum code (for reading a qwic device for instance).

ADC reading is supported for AVR only for now. I think it would be more valuable for the project to merge it as is and rely on subsequent pull requests to implement the missing parts for other controllers.

edit: I've completed the documentation as well, but I could go a little deeper in the circuitry segment. As far as I understand, it should be possible to connect an analog device to any keyboard, but my experience and knowledge is limited to my own keyboard, so validation from an experienced electronician would be required.

@drashna

This comment has been minimized.

Copy link
Member

commented Feb 5, 2019

This is ready to go, correct?

@a-chol

This comment has been minimized.

Copy link
Contributor Author

commented Feb 5, 2019

I just pushed some documentation fixes, I confirm it's ready to go!

@drashna

drashna approved these changes Feb 6, 2019

@drashna

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

There are some merge conflicts here. If you could resolve these?

@a-chol

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

Conflicts are resolved. It's pretty cool to be able to do so from github directly!

@a-chol a-chol force-pushed the a-chol:hid_joystick branch from eba9ccb to 97cdbf4 Apr 11, 2019

add support for hid gamepad interface
add documentation for HID joystick
Add joystick_task to read analog axes values even when no key is pressed or release. update doc
Update docs/feature_joystick.md
Manage pin setup and read to maintain matrix scan after analog read

coredump and others added some commits Dec 7, 2018

Incorporates patches and changes to HID reporting
There are some patches provided by @a-chol incorporated on this commit,
and also some changes I made to the HID Report structure.

The most interesting is the one dealing with number of buttons: Linux
doesn't seem to care, but Windows requires the HID structure to be byte
aligned (that's in the spec). So if one declares 8/16/32... buttons they
should not have any issues, but this is what happens when you have 9
buttons:

```
 bits |0|1|2|3|4|5|6|7|
      |*|*|*|*|*|*|*|*| axis 0 (report size 8)
      |*|*|*|*|*|*|*|*| ...
      |*|*|*|*|*|*|*|*|
      |*|*|*|*|*|*|*|*|
      |*|*|*|*|*|*|*|*|
      |*|*|*|*|*|*|*|*|
      |*|*|*|*|*|*|*|*| axis 6
      |*|*|*|*|*|*|*|*| first 8 buttons (report size 1)
      |*| | | | | | | | last of 9 buttons, not aligned
```

So for that I added a conditonal that will add a number of reports with
size 1 to make sure it aligns to the next multiple of 8. Those reports
send dummy inputs that don't do anything aside from aligning the data.

Tested on Linux, Windows 10 and Street Fighter (where the joystick is
recognized as direct-input)
Add save and restore of each pin used in reading joystick (AVR).
Allow output pin to be JS_VIRTUAL_AXIS if the axis is connected to Vcc
instead of an output pin from the MCU.

Fix joystick report id

Fix broken v-usb hid joystick interface. Make it more resilient to unusual settings (none multiple of eight button count, 0 buttons or 0 axes)

Correct adc reading for multiple axes. Piecewise range conversion for uncentered raw value range. Input, output and ground pin configuration per axis.

Documentation fixes

@a-chol a-chol force-pushed the a-chol:hid_joystick branch from 97cdbf4 to 5939a44 Apr 24, 2019

@drashna

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

Unfortunately, some more merge conflicts have popped up!

include $(DRIVER_PATH)/qwiic/qwiic.mk

ifeq ($(strip $(JOYSTICK_ENABLE)), yes)

This comment has been minimized.

Copy link
@drashna

drashna Jul 30, 2019

Member

Looks like this got added twice?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.