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

ShinyDox merge nuked the Ergodox EZ #3321

Closed
drashna opened this issue Jul 5, 2018 · 5 comments
Closed

ShinyDox merge nuked the Ergodox EZ #3321

drashna opened this issue Jul 5, 2018 · 5 comments

Comments

@drashna
Copy link
Member

drashna commented Jul 5, 2018

The shinydox merge breaks a bunch of stuff for the Ergodox board.

First is that the RGB defaults to underglow off, and matrix on.
Second, is that even when swapping these (Underglow on, and Matrix off), there is an issue with includes and it won't compile right (for instance, my code won't compile, unless I add an include for rgblight.
Third .... not sure if it's fully related, but I can't use ifdef KEYBOARD_ergodox_ez or any other ifdef for my ergodox layout, as it will not detect it, for whatever reason.
Fourth, even when getting RGB underglow working, it is displayed wrong. It no longer bit bangs the color to the left half, and the right half only shows part of the strip lit up... and it's random colors.

So, the changes HARD broke the ergodox board.

I can issue a PR to revert the changes introduced by the hf/shinydox branch, but I'd rather see the issue fixed. (let me know, I'll definitely test!)

Tagging @jackhumbert @ezuk to let you guys know.

@drashna
Copy link
Member Author

drashna commented Jul 5, 2018

As a stop gap, git revert 9c2dde98e2b963704eb7cb87f6c53c52599fba53 -m 1 will fix this issue locally, by reverting the change.

@jackhumbert
Copy link
Member

@drashna It looks like the breaking changes to the RGB stuff are here.

Adding in the rgblight.h include somewhere and fixing the defaults for whatever @ezuk wants would make sense!

@drashna
Copy link
Member Author

drashna commented Jul 9, 2018

You're right about the section. Specifically, it needs these still:

#define RGBW_BB_TWI
#define RGBW 1

It needs both, as well. I'll open a PR and tag ezuk in it.

That said... the ifdef KEYBOARD_ergodox_ez in the make file DOES NOT work. It used to. But now, it doesn't. And the problem is that I was wrapping the rgblight enable with that.

Eg, this does not work:

ifdef KEYBOARD_ergodox_ez
  RGB_MATRIX_ENABLE = no
  RGBLIGHT_ENABLE   = yes
endif

But this does:

  RGB_MATRIX_ENABLE = no
  RGBLIGHT_ENABLE   = yes

I'm not even sure why this would cause this change, as nothing in the code should

@drashna
Copy link
Member Author

drashna commented Jul 9, 2018

....
Wow.
Just for documentation purposes,

ifdef KEYBOARD_ergodox_ez
  RGB_MATRIX_ENABLE = no
  RGBLIGHT_ENABLE   = yes
endif

does not work, but

ifdef KEYBOARD_ergodox_ez
  RGBLIGHT_ENABLE   = yes
  RGB_MATRIX_ENABLE = no
endif

does work just fine.

Makefiles (rules.mk) are still a lot of magic to me, so no idea why this matters (thouglh rgblight and then rgb matrix is the order that common_features lists these in, as well as how the ergodox's rules.mk lists them in too, so ... maybe that's why?).

Either way, I can say that this is closed now.

@drashna
Copy link
Member Author

drashna commented Jul 9, 2018

Wrong. I need to use ifeq ("$(KEYBOARD)","ergodox_ez"), not ifdef KEYBOARD_ergodox_ez

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

No branches or pull requests

3 participants