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

Migrate keyboards using uGFX to LED_MATRIX #9657

Merged
merged 4 commits into from Jun 10, 2021

Conversation

firetech
Copy link
Contributor

@firetech firetech commented Jul 5, 2020

Migrate Input Club keyboards (Ergodox Infinity and Whitefox) away from uGFX for keyboard backlight.

Description

Ergodox Infinity and Whitefox use uGFX to control their keyboard backlight. Because of licensing issues, it's desirable to move away from uGFX (see #5268). The chip in question (IS31FL3731C) already had a working driver for use with LED Matrix (drivers/issi/is31fl3731-simple.c).

Since uGFX is still compiled for the LCD display in Ergodox Infinity, and it caused some naming conflicts, I had to rename the point_t type of LED/RGB Matrix. led_point_t seemed fitting. (This is in conflict with two lines in #12651, but is an easy fix.)

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

@drashna drashna added awaiting_pr Relies on another PR to be merged first core enhancement labels Jul 21, 2020
@drashna drashna requested a review from a team July 21, 2020 00:46
@drashna
Copy link
Member

drashna commented Jul 21, 2020

I suspect that it will need more changes to get this working properly (read: 100%), but it looks like a good start, at least!

@tzarc
Copy link
Member

tzarc commented Jul 21, 2020

Given that it's changing a bunch of things at once, it'll need to go to develop instead of master.

@firetech firetech changed the base branch from master to develop July 21, 2020 08:33
@firetech
Copy link
Contributor Author

firetech commented Jul 21, 2020

I suspect that it will need more changes to get this working properly (read: 100%), but it looks like a good start, at least!

Definitely. For now, I'm waiting for the i2c support to be merged before doing more work on this. I partly put it up here in case I forget, or someone else wants to continue. :)

@drashna
Copy link
Member

drashna commented Jul 24, 2020

Looks like there is an issue with the whitefox board:

Making whitefox with keymap default                                                                    [ERRORS]
lib/ugfx/src/gdisp/gdisp.c: In function '_gdispInit':
lib/ugfx/src/gdisp/gdisp.c:566:4: error: useless type name in empty declaration [-Werror]
    extern GDISPVMTLIST        GDISP_DRIVER_LIST;
    ^~~~~~
cc1: all warnings being treated as errors
 | 
 | 
 | 
make[1]: *** [tmk_core/rules.mk:386: .build/obj_whitefox/lib/ugfx/src/gdisp/gdisp.o] Error 1
make[1]: *** Waiting for unfinished jobs....

@firetech
Copy link
Contributor Author

firetech commented Jul 24, 2020

Looks like there is an issue with the whitefox board:

Yeah, I haven't touched the Whitefox yet, but I have changed some common code in quantum/visualizer, so it's entirely expected that it doesn't compile yet. ;)

Making this posting as a PR might have been jumping the gun a bit, it might have been better to make a normal issue and just refer to my branch...

@fauxpark
Copy link
Member

The I2C PR has been merged now, so this should be good to progress.
Additionally, I just discovered the Infinity60 (also by Input:Club) uses the 3731C too, but has some custom controller code instead of uGFX.

@stale
Copy link

stale bot commented Feb 2, 2021

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.

@fauxpark
Copy link
Member

Yeah, that's an issue with the rest of the ISSI drivers too. It'd probably be best to update them all in one go, this will involve modifying some keyboard-level code as some of them also have SDB hooked up to a GPIO and needed a similar patch:

exclusive/e6_rgb
k_type
melgeek/mj61
melgeek/mj63
melgeek/mj64
melgeek/mj65
melgeek/mojo75
opendeck/32

@firetech
Copy link
Contributor Author

firetech commented May 11, 2021

@fauxpark Do you intend to work on that, or what is the plan?

EDIT: In the meantime, I'll check if setting SDB in matrix_init_kb() instead would work for Ergodox Infinity (although I have a feeling I tried that), and if so, this could be merged before such a rewrite?

@fauxpark
Copy link
Member

@fauxpark Do you intend to work on that, or what is the plan?

Yup, but it might be a short while as technically develop is now locked to new PRs (apart from urgent bugfixes, of course).

@firetech
Copy link
Contributor Author

Updated to be based on #12864 (not including it, but can't be built without it), and moved the IS31FL3731 SDB stuff to keyboard_pre_init_kb() (matrix_init_kb() is called after led_matrix_init(), so that didn't work), in anticipation of an improved, common way of setting SDB.

keyboards/whitefox/whitefox.c Outdated Show resolved Hide resolved
keyboards/whitefox/whitefox.c Outdated Show resolved Hide resolved
keyboards/ergodox_infinity/ergodox_infinity.c Outdated Show resolved Hide resolved
keyboards/ergodox_infinity/ergodox_infinity.c Outdated Show resolved Hide resolved
@firetech
Copy link
Contributor Author

Fixed the changes requested by @fauxpark, and removed some code made unnecessary by #12878.

@fauxpark fauxpark requested a review from a team May 13, 2021 17:18
@firetech firetech mentioned this pull request May 14, 2021
14 tasks
@firetech firetech mentioned this pull request Jun 8, 2021
14 tasks
Comment on lines +81 to +91
#ifdef LED_MATRIX_ENABLE
/*
* Since K20x is stuck with a 32 byte EEPROM (see tmk_core/common/chibios/eeprom_teensy.c),
* and neither led_matrix_eeconfig.speed or .flags fit in this boundary, just force their values to default on boot.
*/
# if !defined(LED_MATRIX_STARTUP_SPD)
# define LED_MATRIX_STARTUP_SPD UINT8_MAX / 2
# endif
led_matrix_set_speed(LED_MATRIX_STARTUP_SPD),
led_matrix_set_flags(LED_FLAG_ALL);
#endif
Copy link
Member

Choose a reason for hiding this comment

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

Ideally, I would like to see if something like #12947 for the K20x fixes this, before merging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as my research has lead me to understand, you basically need to wipe the entire MCU and reflash the bootloader to change the "EEPROM" size. 🙁

Copy link
Member

Choose a reason for hiding this comment

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

Then I'm not sure how else this can be solved if we want VIA support (and room for future core settings)... perhaps we should just fall back to the transient EEPROM implementation, that will eliminate the need for this code at least.

Copy link
Contributor Author

@firetech firetech Jun 8, 2021

Choose a reason for hiding this comment

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

I've done some more investigation and realized that the partitioning code in eeprom_teensy.c isn't actually run on an already partitioned MCU. When I tried increasing EEPROM_SIZE locally, I guess my board therefore just crashed due to the firmware configuration not matching the hardware configuration.

The documentation is a bit fuzzy on the subject, but NXP Support claimed in their forum that the relevant flash and registers need to be in "an erased state" before running the PGMPART command. I'm a bit hesitant to try since I don't want to risk bricking my main keyboard, but it might be possible to try skipping the FTFL->FCNFG & FTFL_FCNFG_RAMRDY if statement and that way force the PGMPART command to be run. Due to what NXP Support said in the forums, my guess is that it won't work.

Edit: Forgot to mention that I have verified that the current config on my board MCU matches the setup that would be created by eeprom_initialize for a 32 byte EEPROM. Skimming the bootloader code made me worried that it expected a different partitioning, but that doesn't seem to be the case.

Copy link
Member

@fauxpark fauxpark Jun 8, 2021

Choose a reason for hiding this comment

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

I've done some more investigation and realized that the partitioning code in eeprom_teensy.c isn't actually run on an already partitioned MCU.

As far as I can tell, the original Kiibohd firmware doesn't do any partitioning (so there is no persistent storage, apparently). So what probably happens when switching from Kiibohd to QMK is the eeprom_initialize() function gets called and sets up the FlexNVM for a 32-byte EEPROM.

This code seems to be mainly intended for actual Teensys, which have an external chip doing the bootloading, and according to the PJRC forum thread mentioned in the other PR, it sends the SETRAM command to turn the EEPROM functionality off. It also likely does a mass erase first.

And yeah, the manual says:

After clearing CCIF to launch the Program Partition command, the flash memory module first verifies that the EEPROM Data Size Code and FlexNVM Partition Code in the data flash IFR are erased.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm. It might even be possible to switch the FlexRAM function over from EEPROM to RAM when an incompatible EESIZE is detected in SIM->FCFG1, in effect making the EEPROM driver work using the same code, but behaving transient. I'm gonna do some experimenting. 👨‍🔬

Copy link
Contributor Author

@firetech firetech Jun 9, 2021

Choose a reason for hiding this comment

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

Working prototype! 😄

I also realized that the "crash" during boot after increasing EEPROM_SIZE was just an infinite loop in flexram_wait() due to the EEERDY flag never being set after an out-of-bounds write. Just making sure the FlexRAM isn't written to outside the partitioned EEPROM size is enough to make the board boot properly. In other words, the entire if (eeprom_size < EEPROM_SIZE) { ... } block in my prototype can be removed (but it might be desired).

If you feel that this would be a valid solution to the problem, I can make a PR of this after #12947 has been merged (would conflict heavily otherwise).

Copy link
Member

Choose a reason for hiding this comment

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

An issue detailing all of these findings may be a good idea, rather than having it all in this tangentially-related PR discussion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point... I'll get one started.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

One last thing, then I think I can merge this :)

keyboards/whitefox/config.h Outdated Show resolved Hide resolved
keyboards/ergodox_infinity/config.h Outdated Show resolved Hide resolved
@firetech
Copy link
Contributor Author

Fixed.

Was considering adding a check of EEPROM_SIZE to the eeprom limit hack in matrix_init_kb discussed above, but realized that EEPROM_SIZE probably isn't defined in that context...

@fauxpark
Copy link
Member

Thanks for all your work!

@fauxpark fauxpark merged commit 6fe3943 into qmk:develop Jun 10, 2021
@firetech firetech deleted the input_club_led_matrix branch July 12, 2021 15:48
mechlovin pushed a commit to mechlovin/qmk_firmware that referenced this pull request Jul 30, 2021
mechlovin pushed a commit to mechlovin/qmk_firmware that referenced this pull request Jul 30, 2021
nhongooi pushed a commit to nhongooi/qmk_firmware that referenced this pull request Dec 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting_pr Relies on another PR to be merged first awaiting review enhancement keyboard
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants