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

rgblight idle timeout feature #6450

Closed
wants to merge 3 commits into from
Closed

rgblight idle timeout feature #6450

wants to merge 3 commits into from

Conversation

nvitaterna
Copy link

Description

This adds an idle timeout features (independent of your PC's sleep settings). In simple terms: if there are no keystrokes after X minutes, turn off the underglow. All you need to do is add #define RGBLIGHT_IDLE_TIMEOUT 10 (any number in minutes) in config.h to enable the feature.

A couple notable things were added as side effects:

  1. Including quantum.h in rgblight.h.
  2. Adding a process_rgblight function to process_record_quantum in quantum.c. Could also be useful if other future features need to hook into this loop. I couldn't find anywhere in rgblight.c where it was already being called.

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.
  • 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).

@nvitaterna
Copy link
Author

The previous PR is here: #5475

The code for this feature has been integrated with the existing rgb light code.

quantum/rgblight.c Outdated Show resolved Hide resolved
quantum/rgblight.c Outdated Show resolved Hide resolved
Copy link
Contributor

@yanfali yanfali left a comment

Choose a reason for hiding this comment

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

Couple of comments, otherwise looks reasonable.

@@ -267,6 +267,7 @@ void rgblight_timer_toggle(void);
// if timeout is set, enable the rgblight idle feature
#ifdef RGBLIGHT_IDLE_TIMEOUT
#define RGBLIGHT_IDLE_ENABLE
#define RGBLIGHT_IDLE_MULTIPLIER 60000
Copy link
Contributor

@yanfali yanfali Jul 31, 2019

Choose a reason for hiding this comment

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

Maybe something like RGB_IDLE_MULTIPLIER_MILLIS

Copy link
Member

Choose a reason for hiding this comment

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

How about RGB_DISABLE_AFTER_TIMEOUT, as we're using that with RGB Matrix:

https://docs.qmk.fm/#/feature_rgb_matrix?id=additional-configh-options

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm ok with that, is just like to know what the unit of measurement is

Copy link
Member

Choose a reason for hiding this comment

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

I'm ... not exactly sure, but the code is called:

bool suspend_backlight = ((g_suspend_state && RGB_DISABLE_WHEN_USB_SUSPENDED) || (RGB_DISABLE_AFTER_TIMEOUT > 0 && g_rgb_counters.any_key_hit > RGB_DISABLE_AFTER_TIMEOUT * 60 * 20));

Copy link
Member

Choose a reason for hiding this comment

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

@XScorpion2 what is the unit in, for RGB_DISABLE_AFTER_TIMEOUT (my brain is failing me)

Copy link
Contributor

Choose a reason for hiding this comment

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

Technically it's 1.2s units. No idea why it was coded like that. Never bothered changing it so it made sense.

Copy link
Author

Choose a reason for hiding this comment

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

This should be changed to seconds or minutes. Documentation says "ticks" which doesn't really mean anything

Copy link
Contributor

Choose a reason for hiding this comment

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

I just submitted a pr for RGB Matrix to clean up it's idle timeout and change it to seconds: #6480

Copy link
Contributor

@XScorpion2 XScorpion2 left a comment

Choose a reason for hiding this comment

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

This is overly complicated for what it does.

@@ -797,6 +805,15 @@ void rgblight_task(void) {
// static light mode, do nothing here
if ( 1 == 0 ) { //dummy
}
#ifdef RGBLIGHT_IDLE_ENABLE
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no need for multiple defines for this feature as you can just used #if RGBLIGHT_IDLE_TIMEOUT > 0

Copy link
Author

Choose a reason for hiding this comment

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

Fair enough. Should I just use the rgb matrix variable or still use a separate one.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would keep the RGB Light define for this feature separate as there has been interest in running both RGB Light & Matrix at the same time.

if (record->event.pressed) {
if (rgblight_idle_timedout) {
rgblight_idle_timedout = false;
rgblight_enable_noeeprom();
Copy link
Contributor

@XScorpion2 XScorpion2 Aug 3, 2019

Choose a reason for hiding this comment

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

Don't change the enable/disable state, as a user could have previously disabled the rgblight, and this would turn it back on against the users wishes. Use a similar pattern to what rgb matrix uses to turn off the LEDs without changing user state.

Copy link
Author

Choose a reason for hiding this comment

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

That makes sense. I will look at the rgb matrix code.

Copy link
Member

Choose a reason for hiding this comment

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

the rgb_matrix code uses a g_suspend_state boolean locally to determine if it's suspended or not, and renders based on that.

@drashna
Copy link
Member

drashna commented Sep 21, 2019

There are a number of merge conflicts here. If you could update the PR to fix these?

@stale
Copy link

stale bot commented Nov 20, 2019

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.

@stale
Copy link

stale bot commented Dec 20, 2019

Thank you for your contribution!
This pull request has been automatically closed because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, or re-open when it's ready.

@stale stale bot closed this Dec 20, 2019
@nvitaterna nvitaterna deleted the rgblight_idle_timeout branch June 24, 2020 22:16
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.

4 participants