Improved OpenRGB protctol for RGB Matrix keyboards

[Kasper24]

This is a major improvement over #10961 - the matrix in OpenRGB is now using the points array to build the matrix, which makes adding more keyboard as easy as OPENRGB_ENABLE = YES in the rules file. There's no need anymore to add additional info for each keyboard

OpenRGB PR

[drashna]

Suggested change

	void matrix_init_user(void)
	{
	//user initialization
	}
	void matrix_scan_user(void)
	{
	//user matrix
	}
	bool process_record_user(uint16_t keycode, keyrecord_t *record)
	{
	return true;
	}

[drashna]

Suggested change

	// Runs just one time when the keyboard initializes.
	void matrix_init_user(void) {
	};
	// Runs constantly in the background, in a loop.
	void matrix_scan_user(void) {
	};

[drashna]

IIRC, this may cause issues.

[Kasper24]

Issues with VIA? If so, maybe only change it to 64 if OpenRGB is enabled in the meantime?

[Kasper24]

@drashna Updated the PR with the changes you asked for. Is this something you guys will be more willing to merge considering there's no need to configure the boards for OpenRGB now?

[tzarc]

At this stage I think I'm okay with the changes as they stand, apart from the question about RAW_EPSIZE. I'm inclined to leave it at 32, mainly because going forward I'd expect that XAP (#11567) would end up being used instead, which has a 64-byte report.

@Kasper24 -- coming from the perspective of using this PR as heavy inspiration for the lighting side of things with XAP, was the implementaton of "zones" something that was inherently useful for UI-side configurability? Wondering if we need to extend the rgb_matrix definitions "properly" beforehand, allowing for zone naming and assignment of LEDs to specific zones. Thoughts?

[CalcProgrammer1]

32 bytes per packet would be rather limiting. The frame rate of effects run from the host is dependent on how many color values we can pack into one USB message. Right now we can pack 20 colors into one 64-byte packet which allows for smooth animations at 60fps as long as the RGB Matrix layer can keep up. I would expect 32 bytes (10 colors, and that's if we only use 2 bytes of overhead) would be significantly slower.

[tzarc]

Then in this case you're going to have to only bump it to 64 when OpenRGB is enabled. Not using OpenRGB shouldn't inadvertently break other people using raw hid, on the off chance that they're reliant on it being 32 bytes long.

[Kasper24]

@tzarc

1. In the updated version, we tried to use the already available info as much as possible. We used the led_flags array to determine how many zones the keyboard has. If we found 1 or more LEDs flagged as underglow, it means (or should) that the keyboard has 2 zones - 1 for key LEDs, 1 for underglow LEDs. We then name it "Keyboard" and "Underglow" respectively. The issue is maybe some keyboards should have more than 2 zones? since the options with QMK are endless, this method might not always be correct for less tradiontal keyboards. The reason we changed it to work this way compared to Implement OpenRGB protctol for RGB Matrix keyboards #10961 is that we couldn't manually configure every QMK keyboard, so it was a trade-off of sorts.

2. OpenRGB and QMK are using 2 different grid systems. That's why in Implement OpenRGB protctol for RGB Matrix keyboards #10961 I had to add the OPENRGB_MATRIX_MAP macro. If you look here, @jath03 added all the functioniallity needed to convert the grid QMK uses to something OpenRGB can use, again to reduce manual configuration. While the QMK method is superiror, other apps might still be doing something closer to what OpenRGB is doing to layout a matrix. The matrix_co array inside the led_config_t struct is similiar to what OpenRGB uses, but it it's following the switch matrix layout, so for keyboards where the above differs from the physical layout (Drop CTRL for example) we couldn't use that.

3. Another issue we had was getting the names from QMK, since the keymap array also contains the "empty' cellls it wasn't as staright-forward. Maybe implenent a method to easily retrive those?

4. Keeping the endpoint size at 32 will be very limiting for RGB animations controllred from the host. If the goal is to eventually replace VIA, why not bump it to highest value possible?
Are we talking about XAP or OpenRGB here? I was under the impression you guys are gonna use it as inspiration for the RGB stuff in the XAP API, but not merge it in?

[tzarc]

@Kasper24

1. Understood, I was asking whether or not you believe it's useful if the RGB matrix definitions were amended for XAP-enabled firmwares, to allow for explicit assignment of things like zones if it helps host applications with UX in general. This obviously would also need the ability to define zone names, as well as being able to query them from the XAP endpoint. It sounds like it would be useful from the perspective of removing ambiguity?
2. Assuming a "clean slate" -- what would your recommendation be in this area? We're currently moving towards a data-driven approach for keyboard definitions, so build-time codegen for things like mappings is absolutely achievable, so long as enough information is present in the info.json file. Alternatively, the next point.....
3. Yes, that would be the intent. At this stage we're looking at embedding a compressed copy of a cut-down version of the info.json. I'm pushing for "layout configurations" where you can swap sections based on user selection -- (ansi or iso enter) for example, with the embedded json containing both physical positions as well as equivalent matrix positions. I'm uncertain if we'd be best suited to use a different mechanism, but it's early stages at this point.

Also, XAP has its own endpoint, and is 64 bytes in length. There is some preamble required to correlate messages with their responses, and at this point the maximum data size is 60 bytes.

[stale]

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

[lbibass]

Any updates on getting this pulled in? Seems it’s just waiting on a review?

[CorePoint]

@lbibass
I'm pretty sure this will never get merged, in favor of XAP.
The protocol could then be used by OpenRGB to communicate with your device.
But there is this fork you could play around with until then.