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

Reduce number of pre-defined Gamepad instances #266

Open
SukkoPera opened this issue Mar 15, 2021 · 14 comments · May be fixed by #316
Open

Reduce number of pre-defined Gamepad instances #266

SukkoPera opened this issue Mar 15, 2021 · 14 comments · May be fixed by #316

Comments

@SukkoPera
Copy link
Contributor

SukkoPera commented Mar 15, 2021

SingleGamepad_ Gamepad1;
SingleGamepad_ Gamepad2;
SingleGamepad_ Gamepad3;
SingleGamepad_ Gamepad4;

extern SingleGamepad_ Gamepad1;
extern SingleGamepad_ Gamepad2;
extern SingleGamepad_ Gamepad3;
extern SingleGamepad_ Gamepad4;

I think it would be better to reduce the number of pre-defined single-report Gamepad instances to 2.

Having 4 does not make much sense IMHO as even on the most capable 32U4 there are only 3 usable endpoints, so besides barring the possibility to use the Gamepad with one or two different devices, the 4 instances cannot even show up all together.

Having two - or even a single one - is not a limitation, since additional devices can be declared in user sketches, if more are needed.

@NicoHood
Copy link
Owner

NicoHood commented Mar 15, 2021

They cannot be declared in the user sketch, since they need a different id, or am I wrong?

But you are right. with CDC Serial only 3 are possible at maximum. But What I suggest is, that we document on how to disable the CDC Serial somewhere in the wiki. Cause I've seen people asking for more controllers, so I guess it is okay to have them.

What is the negative effect of having more than 2? Do they consume more memory? I think not, correct me if I am wrong.

@SukkoPera
Copy link
Contributor Author

SukkoPera commented Mar 15, 2021

Uhm... I cannot test right now, but as the declaration is just a simple SingleGamepad_ GamepadX, I don't see any problems with declaring them in user sketches.

Documenting how to disable the CDC serial is a good idea (there are some hints here btw), but still it sounds odd to me to ship with something that requires fiddling with the Arduino source files to be usable. If someone is smart enough to do that, I'm sure they can also add/uncomment a few lines in the HID Library to enable as many Gamepads as they want (but still, I'm pretty convinced the latter can be done fully in the user sketch).

On the contrary, a beginner expects the library to work out of the box, so maybe they start using a Gamepad, then add a keyboard which won't show up at all, to their surprise. This is because even if only Gamepad1.begin() is called, all the 3 available endpoints will instantly get used by Gamepads.

@NicoHood
Copy link
Owner

This is because even if only Gamepad1.begin() is called, all the 3 available endpoints will instantly get used by Gamepads.

I dont think this is true. If that is the case, we'd have problems since the last 3-4 years or so.

@SukkoPera
Copy link
Contributor Author

SukkoPera commented Mar 15, 2021

Exactly, I tested it and that's my experience.

That's what's causing the problems in #139, I suspect (My logs are in that issue).

@NicoHood
Copy link
Owner

Can this be solved by creating extra files with:

SingleGamepad1.cpp (and 2,3,4)

#include "SingleGamepad.h"
SingleGamepad_ Gamepad1; // and 2,3,4

Then the compiler should only create those instances if they are referenced.

@SukkoPera
Copy link
Contributor Author

Uhm... I cannot test right now, but as the declaration is just a simple SingleGamepad_ GamepadX, I don't see any problems with declaring them in user sketches.

I have tested this and it works fine. Still haven't had the time to test the rest though.

@alijani1
Copy link

Please dont disable the extra end points. I also think documenting how to disable cdc is best.

I also would love to see the multi-report endpoint working a multiple gamepads. why limit the multi-report end point to just one gamepad?

@NicoHood
Copy link
Owner

@alijani1 I think (but its been a long time) that it just did not work under windows in multireports.

@SukkoPera
Copy link
Contributor Author

SukkoPera commented Mar 24, 2021

@alijani1 Can you help testing what @NicoHood proposed above?

If that doesn't work, I am still convinced only 1/2 instances should be predefined. Then we would have docs saying "you can have up to 5 if you do the following: 1. disable cdc; 2. enable more in the lib", with the latter that could be made into just enabling a #define in HID-Settings.h.

The point is: not everybody wants to use 5 gamepads or 0. For instance a board of mine needs 2 gamepads, keyboard and consumer. As it is now, as soon as I init the first gamepad, all the rest disappears.

@alijani1
Copy link

@alijani1 I think (but its been a long time) that it just did not work under windows in multireports.

Perhaps it was windows 7 or 8. I use other libraries with windows 10 and multi-report gamepad descriptors work fine on Windows 10. In linux there is an issue as Linux only supports multiple HIDs on a single usb natively only if the quirk flag HID_QUIRK_MULTI_INPUT is set in its hid-ids list. hid-quriks.c has all the VID/PID combinations that have the flag set. if you dont use one of the preset ones, you have to set the usb quirk flag manually in start up script.

@alijani1
Copy link

@alijani1 Can you help testing what @NicoHood proposed above?

I will try, but I know in my case, I was able to use keyboard HID functions even after I do gamepad1.begin. I have to double check as I may have disabled gamepad3 in that project. Will let you know once I get to review and also try singlegamepad extra files.

@SukkoPera
Copy link
Contributor Author

SukkoPera commented Aug 29, 2021

Can this be solved by creating extra files with:

SingleGamepad1.cpp (and 2,3,4)

#include "SingleGamepad.h"
SingleGamepad_ Gamepad1; // and 2,3,4

Then the compiler should only create those instances if they are referenced.

I have finally tested this myself - not thoroughly though - and it seems to work fine. PR submitted (shouldn't hurt in any case).

@SukkoPera
Copy link
Contributor Author

It looks like the ability to disable the CDC serial has finally been merged into the Arduino mainline. Let's hope for a release soon.

@fermino
Copy link

fermino commented Mar 15, 2024

@NicoHood would you be willing to accept a PR that allows disabling the automatic creation of instances via a compile-time flag? (I am in the current need of defining the instances myself and I'm patching it manually but if it might help others I can submit it!)
Thanks :)

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 a pull request may close this issue.

4 participants