Convert clipping variables in rgblight.c to a structure

[drashna]

This allows these variables to be called by keyboard code that uses the custom rgb driver.

Right now, any keyboard using a custom rgblight driver cannot access these settings, so any benefit from using them is lost for those boards. Such as the Ergodox EZ. This allows them to be externed, so they can be used by those boards.

Types of Changes

• Core
• Bugfix
• New feature
• Enhancement/optimization
• Keyboard (addition or update)
• Keymap/layout/userspace (addition or update)
• Documentation

Checklist

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• I have tested the changes and verified that they work and don't break anything (as well as I can manage).

[fauxpark]

#pragma weak

[mtei]

FYI:
I think it's better to put those variables in a structure.

Like below:

diff --git a/quantum/rgblight.h b/quantum/rgblight.h
index e3aa098e4..de90319db 100644
--- a/quantum/rgblight.h
+++ b/quantum/rgblight.h
@@ -170,6 +170,14 @@ typedef struct _rgblight_status_t {
 #    endif
 } rgblight_status_t;
 
+typedef struct _rgblight_ranges_t {
+    uint8_t clipping_start_pos;
+    uint8_t clipping_num_leds;
+    uint8_t effect_start_pos;
+    uint8_t effect_end_pos;
+    uint8_t effect_num_leds;
+} rgblight_ranges_t;
+
 /* === Utility Functions ===*/

diff --git a/quantum/rgblight.c b/quantum/rgblight.c
index 7949bb688..bd9ea6814 100644
--- a/quantum/rgblight.c
+++ b/quantum/rgblight.c
@@ -95,11 +95,7 @@ LED_TYPE led[RGBLED_NUM];
 #    define LED_ARRAY led
 #endif
 
-static uint8_t clipping_start_pos = 0;
-static uint8_t clipping_num_leds  = RGBLED_NUM;
-static uint8_t effect_start_pos   = 0;
-static uint8_t effect_end_pos     = RGBLED_NUM;
-static uint8_t effect_num_leds    = RGBLED_NUM;
+rgblight_ranges_t ranges = { 0, RGBLED_NUM,  0, RGBLED_NUM, RGBLED_NUM };

[drashna]

It's a lot more changes that I would like to make here, but I think you're right. It would make things easier, in general. for sure.

[mtei]

FYI:

I think there are several ways.

Export rgblight_ranges directly:

/* File:rgblight.h */
extern rgblight_ranges_t rgblight_ranges;

Prepare a function that returns a pointer to rgblight_ranges:

/* File:rgblight.h */
extern rgblight_ranges_t *rgblight_get_ranges(void);

/* File:rgblight.c */
rgblight_ranges_t *rgblight_get_ranges(void) {
  return &rgblight_ranges;
}

Prepare a new variant of rgblight_set():

/* File:rgblight.c */

__attribute__((weak))
void rgblight_set_with_range(rgblight_ranges_t *range) {
   ...
}

__attribute__((weak))
void rgblight_set(void) {
   rgblight_set_with_range(&rgblight_ranges);
}

[drashna]

@mtei I think, for now, I'm going to leave this as it is. Maybe clean up the formatting and such. And leave the rest to somebody like you, that's a lot more familiar with this code.

The changes that you propose sound good, but ... I'm not confident enough in my skill and comprehension for much more than this.

[mtei]

I'm not sure why RGBLIGHT_CUSTOM_DRIVER was needed.

For example, isn't it possible to override only the output to LEDs as shown below?

diff --git a/quantum/rgblight.c b/quantum/rgblight.c
index 7949bb688..322982cb2 100644
--- a/quantum/rgblight.c
+++ b/quantum/rgblight.c
@@ -596,6 +596,11 @@ void rgblight_sethsv_master(uint8_t hue, uint8_t sat, uint8_t val) { rgblight_se
 void rgblight_sethsv_slave(uint8_t hue, uint8_t sat, uint8_t val) { rgblight_sethsv_range(hue, sat, val, (uint8_t)RGBLED_NUM / 2, (uint8_t)RGBLED_NUM); }
 #endif  // ifndef RGBLIGHT_SPLIT
 
+__attribute__((weak))
+void rgblight_setrawleds(LED_TYPE *start_led, uint16_t num_leds) {
+    ws2812_setleds(start_led, num_leds);
+}
+
 #ifndef RGBLIGHT_CUSTOM_DRIVER
 void rgblight_set(void) {
     LED_TYPE *start_led;
@@ -620,7 +625,7 @@ void rgblight_set(void) {
 #    else
     start_led = led + clipping_start_pos;
 #    endif
-    ws2812_setleds(start_led, num_leds);
+    rgblight_setrawleds(start_led, num_leds);
 }
 #endif

[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.

[mtei]

@drashna
I tried to make a sample patch without using RGBLIGHT_CUSTOM_DRIVER. Does this do what you want?

14bbe50
9e7f65c

[spidey3]

I did some testing on this, and it does seem to be working well.
It looks like there are a few keyboards where work is needed to make them compile after this refactoring. Is there any way that I can help?

[drashna]

@spidey3 if you wanted to do the work for the other boards, you can check out my branch, and open a PR to it. That would/should give you partial/co-author credit for the PR.

[spidey3]

@spidey3 if you wanted to do the work for the other boards, you can check out my branch, and open a PR to it. That would/should give you partial/co-author credit for the PR.

Sure, I can do that.
What's the best git-fu to create that checkout?

[drashna]

The simple way should be:

git fetch origin refs/pull/7720/head
git checkout -b drashna/rgblight/clipping_range FETCH_HEAD

The more complicated way is using hub, or adding my repo as a remote, check out a new branch based on mine.

[drashna]

Depending on what they're doing, rgblight_call_driver passes on a LED array, allows further modification of that array and for to be handled properly.

the rgblight_set_user ... I don't think will do anything that you can't do with rgblight_call_driver.

For instance, the Ergodox EZ is a great example of this, since it sends the LED info over i2c to half of LEDs, and can be mirrored, duplicated or fully addressable.

The other boards I looked at (the lfkeyboard stuff, the noah, and the v60_type_r, these boards could use the rgblight_call_driver function instead. I know, because I tried that before figuring out the source of the issue. And should even work with @MxBlu's mxss board.

However, I don't think that removing the rgblight custom driver setting is appropriate for this PR, just that this makes it possible. I'd rather get this merged, and then work on getting the other boards off of the custom driver code.

I'm fine with changing the ergodox EZ changes here, because I'm the official maintainer for it, and have verified that it works properly.