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

Fix Bug with (RGB|LED)_DISABLE_WHEN_USB_SUSPENDED define #13060

Merged
merged 2 commits into from
Jun 8, 2021

Conversation

drashna
Copy link
Member

@drashna drashna commented Jun 1, 2021

Description

The check actually errors out if not set to anything (just defined, per docs)

Types of Changes

  • Core
  • Bugfix

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

@drashna drashna changed the base branch from master to develop June 5, 2021 16:01
Copy link
Member

@fauxpark fauxpark left a comment

Choose a reason for hiding this comment

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

Should be done for LED Matrix as well?

@drashna
Copy link
Member Author

drashna commented Jun 6, 2021

Should be done for LED Matrix as well?

Yes. But I was having issues with it on RGB matrix, so didn't want to do that yet. Will add now.

@drashna drashna changed the title Fix Bug with RGB_DISABLE_WHEN_USB_SUSPENDED define Fix Bug with *_DISABLE_WHEN_USB_SUSPENDED define Jun 6, 2021
@drashna drashna changed the title Fix Bug with *_DISABLE_WHEN_USB_SUSPENDED define Fix Bug with (RGB|LED)_DISABLE_WHEN_USB_SUSPENDED define Jun 6, 2021
@drashna drashna changed the base branch from develop to master June 7, 2021 20:10
Copy link
Member

@fauxpark fauxpark left a comment

Choose a reason for hiding this comment

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

Out of curiosity, is there a particular reason to use 1 instead of true?

Looking at current master, I don't see any usages of this define that set it to anything other than true or false, except for the docs. So I think they should be updated as well.

Also, IMO the presence of the define itself should be the decider - but that I guess would be a job for develop.

@drashna
Copy link
Member Author

drashna commented Jun 8, 2021

Well, the point is to have define enable the behavior, without the need to have true or false set.

However, having it set to true or false causes issues. And checking if it's equal to true or false causes issues when it's not. At least in the preprocessor stuff.

So, right now, if you use #define RGB_DISABLE_WHEN_USB_SUSPENDED as per the docs, you get:

Compiling: quantum/rgb_matrix.c                                                                    quantum/rgb_matrix.c:70:36error: operator '!=' has no left operand
 #if RGB_DISABLE_WHEN_USB_SUSPENDED != false

Changing it to look for true generates the same issue. This method is the only way I've seen that actually works, and without having to change every define in the repo.

@drashna
Copy link
Member Author

drashna commented Jun 8, 2021

Which is to say that I screwed up, and didn't test the original change enough. This change should allow it to compile correctly, and respect the intended behavior with all three possible defines:

#define RGB_DISABLE_WHEN_USB_SUSPENDED
#define RGB_DISABLE_WHEN_USB_SUSPENDED true
#define RGB_DISABLE_WHEN_USB_SUSPENDED false

Later, I can clean up stuff, on develop, to make it all nice and uniform.

@fauxpark fauxpark merged commit 9975e17 into qmk:master Jun 8, 2021
@drashna drashna deleted the bug/rgb_matrix branch June 9, 2021 01:38
HokieGeek pushed a commit to HokieGeek/qmk_firmware that referenced this pull request Jul 11, 2021
wox pushed a commit to wox/qmk_firmware that referenced this pull request Aug 14, 2021
nhongooi pushed a commit to nhongooi/qmk_firmware that referenced this pull request Dec 5, 2021
BorisTestov pushed a commit to BorisTestov/qmk_firmware that referenced this pull request May 23, 2024
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.

2 participants