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

[Feature Request] RGB_MATRIX feature for split keyboards #9619

Closed
2 of 9 tasks
tyalie opened this issue Jul 1, 2020 · 8 comments
Closed
2 of 9 tasks

[Feature Request] RGB_MATRIX feature for split keyboards #9619

tyalie opened this issue Jul 1, 2020 · 8 comments
Labels
enhancement help wanted stale Issues or pull requests that have become inactive without resolution.

Comments

@tyalie
Copy link

tyalie commented Jul 1, 2020

As of now split keyboards don't support rgb_matrix out of the box. The nature of the problem lies in
the asynchron internal timers of both halves, the disabled keypress evaluation on the slave side, ...

Making it work seamlessly is the goal of the current repo, but there're a few problems to solve first.

Feature Request Type

  • Core functionality
  • Add-on hardware support (eg. audio, RGB, OLED screen, etc.)
  • Alteration (enhancement/optimization) of existing feature(s)
  • New behavior

Description

As mentioned before rgb matrix doesn't work together with split keyboards. There're some forks (#5998 and an iteration on that: #9613) that seem promising, but aren't perfect either. Major problems that need to be fixed first are:

  • how are rgb matrix information transferred

    • both Split Keyboard RGB Matrix Support #5998 and RGB Matrix Support for Split Keyboards #9613 came to the same conclusion, that sending all rgb data to the slave side, is not feasible when using a serial connection. The data rates too slow for that on serial and it as such can introduce a host of other problems like lags, ... As such both concepts gave the slave a render job, which only works synced, if both keyboards have the same firmware
  • synchronization between master and slave timer (see [Feature Request] Timers between two halves of a split keyboard are not synced #9665)

  • how is it guaranteed that both slave and master are in the same state

    • in Split Keyboard RGB Matrix Support #5998 both halves were responsible to interpret the key codes correctly. As there was no rgb matrix state sync between both halves, they could get out of sync pretty fast (especially when one half is operated without the other one attached)
    • in RGB Matrix Support for Split Keyboards #9613 the approach was much different. All logic regarding matrix state selection was responsibility of the master and it only transferred any config changes to the slave. The way here is very similar to the sync implemented in rgblighting.
    • Problems can occur, when packages are incorrectly send and states cannot be synchronized accordingly (which can happen surprisingly often)
  • reactive rgb matrix effects on slave side

    • this would require slave side key evaluation, which can come with a host of other problems. Those include evaluation of RESET, change of rgb matrix states and async behaviour accordingly, ...
      • One solution for it would be to give the slave an empty keymaps with only KC_NO (or all access to it returns KC_NO). This approach was taken in RGB Matrix Support for Split Keyboards #9613. But this has the side effect, that the slave still has no idea what layer the keyboard is currently on. A component that could be implemented easily, depending on the approach taken
  • processing rgb_matrix_indicator_user()

    • As RGB Matrix doesn't limit itself on only indicating layers, but instead uses the function rgb_matrix_indicator_user() that is run each cycle and cull pull information to set the color of single keys from arbitrary places a simple indicator, which layer is used might not be enough.
@tyalie
Copy link
Author

tyalie commented Jul 1, 2020

Suggestions are welcome

@tzarc
Copy link
Member

tzarc commented Jul 1, 2020

Probably need layer state sync too, in order to deal with lighting layers? (Is that RGBLIGHT-only?)

@tyalie
Copy link
Author

tyalie commented Jul 2, 2020

I haven't really looked into the matter on the RGB Matrix side, but I'm not even sure RGB Matrix has something like that without some hacks. At least the rgb_matrix.h doesn't mention anything to my knowledge

Probably what #717 was for.

@sigprof
Copy link
Contributor

sigprof commented Jul 2, 2020

Probably need layer state sync too, in order to deal with lighting layers? (Is that RGBLIGHT-only?)

Yes, lighting layers are currently implemented only in the rgblight code, not in rgbmatrix; and the state of rgblight layers is already synced, so they technically should not need anything more.

Actually there might be some conflicts if we attempt to sync the layer state to the slave side (which would presumably call layer_state_set_user() to update various kinds of indicators) and at the same time send the rgblight layer state there directly.

The RGB Matrix code, on the other hand, has rgb_matrix_indicators_user(), which works in a completely different way (it can pull information for various places to update the LED state), and it would need layer state sync if the user wants some LEDs to show the layer state.

@tyalie
Copy link
Author

tyalie commented Jul 2, 2020

Hmh. I haven't thought about that. But rgb_matrix_indicator_user() could be very ugly to implement. I'll add it to the list.

@stale
Copy link

stale bot commented Oct 4, 2020

This issue has been automatically marked as stale because it has not had activity in the last 90 days. It will be closed in the next 30 days unless it is tagged properly or other activity occurs.
For maintainers: Please label with bug, in progress, on hold, discussion or to do to prevent the issue from being re-flagged.

@stale stale bot added the stale Issues or pull requests that have become inactive without resolution. label Oct 4, 2020
@stale stale bot removed the stale Issues or pull requests that have become inactive without resolution. label Oct 4, 2020
@drashna
Copy link
Member

drashna commented May 26, 2021

Removing the "in progress" tag, so that this can be closed.

develop (as of May 25th) has everything in place for this to work, now. And in a week, the code will hit master. (as well as the removal of the "legacy" corne revision)

@stale
Copy link

stale bot commented Aug 25, 2021

This issue has been automatically marked as stale because it has not had activity in the last 90 days. It will be closed in the next 30 days unless it is tagged properly or other activity occurs.
For maintainers: Please label with bug, in progress, on hold, discussion or to do to prevent the issue from being re-flagged.

@stale stale bot added the stale Issues or pull requests that have become inactive without resolution. label Aug 25, 2021
@tzarc tzarc closed this as completed Aug 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement help wanted stale Issues or pull requests that have become inactive without resolution.
Projects
None yet
Development

No branches or pull requests

4 participants