Possible bug/user inconvenience in handling the maximum number of banks/profiles.

[luziferius]

The current maximum number of supported profiles is stored here as const int MAX_PROFILE = 3;

sidewinderd/src/core/keyboard.hpp

Line 29 in b167c93

const int MAX_PROFILE = 3;

It is used in keyboard.cpp to create the macro storage folders for each profile. This is not a problem.
Just for reference, exactly here:

sidewinderd/src/core/keyboard.cpp

Line 223 in b167c93

for (int i = MIN_PROFILE; i < MAX_PROFILE; i++) {

The actual problem is that this constant originates from sidewinder.hpp and is still used with a different purpose in sidewinder.cpp, here:

sidewinderd/src/vendor/microsoft/sidewinder.cpp

Line 43 in b167c93

profile_ = (profile_ + 1) % MAX_PROFILE;

This will lead to a bug as soon as another keyboard is added that offers >= 4 profiles. MAX_PROFILE will be increased and the code in sidewinder.cpp will break without notice, because it depends on that constant being exactly 3.
The profile rotation for the Sidewinder keyboard should always be 1 → 2 → 3 → 1.
Let MAX_PROFILE be redefined to 10. Then the profile rotation on the sidewinder keyboard will become 1 → 2 → 3 → 3 → 3 → 3 → 3 → 3 → 3 → 3 → 1, which I consider as unintended.

Possible fix for that possible bug:
Use a constant per keyboard, so that adding new keyboards won’t break old ones.
MAX_PROFILE in keyboard.hpp must then be set to the maximum profile number across all supported keyboards.

[tolga9009]

Hi there!

Very good find! Setting a constant per keyboard makes sense to me, I agree on that. I also feel like, we shouldn't create storage folders in Keyboard::Keyboard() constructor. Eventhough it's a common task, we shouldn't create MAX_PROFILE number of folders in the constructor for each keyboard, or atleast not in the same fashion as of right now. Some keyboards might not support it and we might only clutter the user's profile folder with empty folders.

Furthermore, MAX_PROFILE doesn't need to represent the maximum number of profiles across all keyboards. It could be more like a soft cap or fallback, we think is reasonable. E.g. after a quick Google search, I was able to find the Roccat Isku, which supports up to 5 profiles. That's highest number I've come across as of now. We could set MAX_PROFILE to that, with the option to increase the number, whenever needed.

Alternatively, we could scratch MAX_PROFILE and only use per keyboard constants. Probably the best option.

Cheers,
Tolga

[luziferius]

Maybe use a constant class member function instead of a constant variable. Something like
virtual unsigned int number_supported_profiles() = 0; in Keyboard class. It must be implemented in every subclass, like return 3;
This forces the presence of a keyboard-specific constant, maybe even making that number configurable. (Does the hardware allow lighting more than one profile LED? Then we could allow binary encoding 7 or 8 profiles in 3 LEDs, if the user wishes to have that (7 if at least one LED must be on.)

at least we can determine the number of to be created profile folders for each supported keyboard that way.

[tolga9009]

I was planning to move the profile creation code to its own function e.g. Keyboard::CreateFolders(int profileCount)and call it from e.g. SideWinder::SideWinder() constructor, passing along SW_PROFILE_COUNT. This would also help in future situations, where a keyboard doesn't support extra / macro keys and only needs support by sidewinderd e.g. for configuring RGB or whatever.

I think it depends on the hardware, whether setting multiple LEDs at once is supported or not. For the SideWinder X4, X6 and the Logitech G710, I've tried it out personally and can confirm it works. However, I don't have plans to implement such a feature in master branch at the moment. Maybe we can do it in the future, when e.g. a GUI makes such a feature more accessible.

I think auto profile is a more useful feature in that regard. It enables you to have profile sets per application, which automatically switch, as soon as you switch the application. A global profile set is applied, as soon as your active application doesn't have a profile.