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

Initial PoC for Dynamic lighting support #22296

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from
Draft

Initial PoC for Dynamic lighting support #22296

wants to merge 2 commits into from

Conversation

zvecr
Copy link
Member

@zvecr zvecr commented Oct 19, 2023

Description

Still early days, but is somewhat functional and highlights a few areas that need decisions.

Discussion points

  • "Overlay" vs "Effect"
    • Effect would have its buffer allocated at all times, so why not add additional use?
  • Expanded integration into other lighting frameworks?
    • LED Matrix
      • Zero supported mono devices detailed anywhere to use as reference
      • Spec seem to somewhat suggest mapping as a single channel 26.9 Color Attributes?
        • r:255 g:0 b:0 i:1?
          • Windows 11 still shows and sends RGB config for the device
        • r:1 g:0 b:0 i:255?
          • Windows 11 shows no device
        • would require extra validation/OOB checks for each channel in range/multi updates
    • RGBLight - no position data
      • map as a single LED?
      • assume horizontal position?
      • require migration to rgb matrix?
    • Defer to later?
  • splits?
    • sharing buffers is expensive
      • send range/multi updates?
      • Can now sync queue?
      • sync whole buffer periodically to ensure in step?
    • Defer to later?
  • Spec compliance?
    • Do we care about every failure case?
    • Where best to document accepted exceptions?
    • Compliant implementation requires double buffering via complete flag
      • If Effect - 2nd Copy could just be the drivers?
        • Pushes additional render process management within effect
          • render always runs even for static effects - not just a single cycle
          • rendering would have to be decoupled so that another component can drive the process
      • Ignore and partial updates would be visible?
        • Lower RAM - opt in/out?
  • Device type as only keyboard?
    • Allowing override?
    • Mouse would then enable reporting of buttons but removing basic + mods
    • Added config option and additional keycode filtering
  • Measure Flash/RAM usage?
  • Assumed positional data
    • Provided user hooks for overriding?
      • Not DD - could start introducing too much keyboard level code?
  • Zero implementation for vusb and arm_atsam platforms
    • One has limited resources, the other is due for removal
  • Win10 limitations
    • hidapi - Lamp usage missing
    • Win API - foreground only
    • Advertise exact same functionality over XAP?

TODO:

  • Implement interrupt safe request/response
  • Add docs
  • Add link to test client
PoC rgblight patch
diff --git a/quantum/lamparray.c b/quantum/lamparray.c
index 76e9836efa..6616171591 100644
--- a/quantum/lamparray.c
+++ b/quantum/lamparray.c
@@ -113,6 +113,68 @@ void lamparray_backing_update_finished(void) {
     rgb_matrix_overlay_flush();
 }
 #endif
+#ifdef RGBLIGHT_ENABLE
+#    include "rgblight.h"
+
+#    define LAMPARRAY_RED_LEVELS 255
+#    define LAMPARRAY_GREEN_LEVELS 255
+#    define LAMPARRAY_BLUE_LEVELS 255
+#    define LAMPARRAY_INTENSITY_LEVELS 1
+#    define LAMPARRAY_LAMP_COUNT RGBLED_NUM
+#    define LAMPARRAY_UPDATE_INTERVAL (5 * 1000)
+
+/**
+ * \brief Get feature specific lamp info.
+ *
+ * Scales the LED config with the assumed RGB Matrix dimensions (224x64), for simplicity, as a completely flat device.
+ * Assumes all keys are either on the top or bottom of the resulting rectangle.
+ */
+__attribute__((weak)) void lamparray_get_lamp_info_data(uint16_t lamp_id, lamparray_attributes_response_t* data) {
+    data->position.x = (LAMPARRAY_WIDTH / RGBLED_NUM / 2) + (LAMPARRAY_WIDTH / RGBLED_NUM * lamp_id);
+    data->position.y = LAMPARRAY_HEIGHT / 2;
+    data->position.z = LAMPARRAY_DEPTH;
+    data->purposes   = LAMP_PURPOSE_ACCENT;
+}
+
+/**
+ * \brief Query a HID usage for a given lamp
+ */
+__attribute__((weak)) uint8_t lamparray_get_lamp_binding(uint16_t lamp_id) {
+    return 0;
+}
+
+static bool is_enabled = false;
+static uint8_t bleh_count = 0;
+static rgb_led_t led_overlay[RGBLED_NUM];
+
+void lamparray_backing_enable(bool enable) {
+    is_enabled = enable;
+}
+
+void lamparray_backing_set_item(uint16_t index, lamp_state_t color) {
+    led_overlay[index] = (rgb_led_t){ .r = color.red, .g = color.green, .b = color.blue };
+}
+
+void lamparray_backing_update_finished(void) {
+    bleh_count++;
+}
+
+void housekeeping_task_user(void) {
+    static bool last_bleh_count = 0xFF;
+    if (bleh_count != last_bleh_count) {
+        last_bleh_count = bleh_count;
+        rgblight_set();
+    }
+}
+
+void rgblight_call_driver(rgb_led_t *start_led, uint8_t num_leds) {
+    if (is_enabled) {
+        ws2812_setleds(led_overlay, RGBLED_NUM);
+    } else {
+        ws2812_setleds(start_led, num_leds);
+    }
+}
+#endif
 
 static uint16_t cur_lamp_id   = 0;
 static bool     is_autonomous = true;

Types of Changes

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

Issues Fixed or Closed by This PR

Checklist

  • My code follows the code style of this project: C, Python
  • I have read the PR Checklist document and have made the appropriate changes.
  • 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).

tmk_core/protocol/chibios/usb_main.c Outdated Show resolved Hide resolved
tmk_core/protocol/chibios/usb_main.c Outdated Show resolved Hide resolved
tmk_core/protocol/lufa/lufa.c Outdated Show resolved Hide resolved
quantum/lamparray.c Outdated Show resolved Hide resolved
@Xelus22
Copy link
Contributor

Xelus22 commented Oct 19, 2023

FWIW I do have #22119, and it seems cleaner to have the driver itself do the double buffering, then it could be changed from a void -> bool is31fl3741_update_pwm_buffers to indicate if transfer was success?

@zvecr
Copy link
Member Author

zvecr commented Oct 19, 2023

FWIW I do have #22119, and it seems cleaner to have the driver itself do the double buffering

In many aspects it is not cleaner. It then couples the components even tighter, and following the existing code patterns, would introduce another wall of ifdefs. Then there is the fragmentation of such driver specific implementations, which can introduce inconsistency and make the code harder to diagnose. Also splits would need to share the driver specific buffer for the implementation to be compliant. (It also doesnt to solve the additional implementation requirements i've briefly summarised in the description). Resource usage is potentially already an issue on some boards, its potentially something QMK has to deviate on and not adhere to the spec.

The storage and render area of the eventual solution, is maybe the part that needs the most investigation to work out direction. Though I plan to work through the interrupt issues mentioned (less implementation and more concepts) as the feature could go in with just a user implemented weak backing. And then jump back to this area maybe next week.

tmk_core/protocol/usb_descriptor.h Outdated Show resolved Hide resolved
tmk_core/protocol/usb_descriptor.h Outdated Show resolved Hide resolved
quantum/lamparray.c Outdated Show resolved Hide resolved
quantum/lamparray.c Outdated Show resolved Hide resolved
Comment on lines 67 to 68
data->position.x = (LAMPARRAY_WIDTH / 224) * g_led_config.point[lamp_id].x;
data->position.y = (LAMPARRAY_HEIGHT / 64) * (64 - g_led_config.point[lamp_id].y);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this should take RGB_MATRIX_CENTER into account (by assuming that the boundaries are 2x the specified center coordinates).

quantum/lamparray.c Outdated Show resolved Hide resolved
@sigprof
Copy link
Contributor

sigprof commented Oct 20, 2023

  • r:1 g:0 b:0 i:255?
    • Windows 11 shows no device

Did you also set data->is_programmable = 0 for this test?
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants