Update AW20216S driver and configuration for GMMK/Pro keyboard

[glorious-qmk]

Description

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

[biosmanager]

Could you elaborate where to find documentation for that chip?
The official awinic website does not list the AW20216S.

[tzarc]

Some rationale on why this should be the preferred implementation compared to the already existing driver would be good, too.

[Gigahawk]

Well it looks like it's taking advantage of register autoincrement in AW20216S_write_pwm_buffer().
I deliberately chose to not implement that for simplicity, but I don't think it's a problem either way.
This implementation might be slightly less memory efficient since it allocates enough buffer space to address every register of each driver.

In my own use, the existing implementation doesn't have any noticeable lag, and high speed footage indicates that a full matrix update on the GMMK Pro takes 0.015ish seconds.

[jaqb]

This implementation includes correct (and crucial) delays, one in particular. (US_POR_DELAY) Also in my opinion the code is more legible, but that's my personal preference. I like some things in both implementations. Another advantage of this implementation (presumably because of mcuconf.h) is that the compiled firmware is much smaller. (around 7k smaller)

[petsche]

Not related to the driver implementation, but FWIW the merged keyboard had some reactive effects that did not work quite right for some keys (specifically the Down Arrow). The driver code looked so similar that instead of trying to debug the LED config (which may be trivial) I simply replaced the submitted pro.c with the one in this PR and all the matrix effects seem to be working for all keys now.

Thanks everyone for writing and reviewing the GMMK Pro QMK firmware.

[Gigahawk]

Ah ok, I hadn't tested the reactive effects much, which effect were you using, and was it just the down key?

[petsche]

Ah ok, I hadn't tested the reactive effects much, which effect were you using, and was it just the down key?

I think it was one of the SPLASH effects (turned on with "#define RGB_MATRIX_KEYPRESSES" in config.h). IIRC pressing the down arrow made it seem like the splash happened near the ESC key. I'm not sure about any other keys because I tested out this PR's version shortly after and stuck with it. Not sure I would ever leverage it, but this pro.c marked modifier flags in the LED config so that is something to consider in the merged version, too.

[glorious-qmk]

Is there anything I can do to finish the merge?

[tzarc]

There are conflicts with the existing driver.

[glorious-qmk]

The driver AW20216s is new, which existing driver do you mean?

[tzarc]

A community member already implemented a driver for the LED driver. You've both modified the same files, generating conflicts, which GitHub helpfully lists below. You'll need to rectify those conflicts before anything can progress.

[glorious-qmk]

The led driver implemented was a placeholder based on the IS31FL3731.

[glorious-qmk]

The changes on the driver PR are on files aw20216s.c|h. This driver uses the SPI protocol.
The changes on files 'common_features.mk', 'rgb_matrix.h' and 'rgb_matrix_drivers.c' are to include the driver in the list of drivers.
Finally, there are changes in the keyboard 'gmmk/pro'.

[drashna]

The changes on the driver PR are on files aw20216s.c|h. This driver uses the SPI protocol.
The changes on files 'common_features.mk', 'rgb_matrix.h' and 'rgb_matrix_drivers.c' are to include the driver in the list of drivers.
Finally, there are changes in the keyboard 'gmmk/pro'.

That may be, but because both PRs touched the same files, there are merge conflicts in this one, since the other was merged.

You'd need to resolve the merge conflicts, either by using the web interface and the "Resolve Conflicts" button below, or by rebasing the branch and fixing the merge conflicts there.

Short of doing one of these, the PR won't get reviewed and won't be merged.

[drashna]

I think it was one of the SPLASH effects (turned on with "#define RGB_MATRIX_KEYPRESSES" in config.h). IIRC pressing the down arrow made it seem like the splash happened near the ESC key. I'm not sure about any other keys because I tested out this PR's version shortly after and stuck with it. Not sure I would ever leverage it, but this pro.c marked modifier flags in the LED config so that is something to consider in the merged version, too.

IIRC, this is an issue with how the animations are implemented, and not specific to the GMMK Pro

[Gigahawk]

I think it was one of the SPLASH effects (turned on with "#define RGB_MATRIX_KEYPRESSES" in config.h). IIRC pressing the down arrow made it seem like the splash happened near the ESC key. I'm not sure about any other keys because I tested out this PR's version shortly after and stuck with it. Not sure I would ever leverage it, but this pro.c marked modifier flags in the LED config so that is something to consider in the merged version, too.

IIRC, this is an issue with how the animations are implemented, and not specific to the GMMK Pro

Nah, this was me screwing up the LED-switch mapping, fixed in #13189 (original mapping was generated by a bunch of python spaghetti).

Unless this is referring to the splash animation moving from one side of the matrix to the other, which would be a QMK problem, but I don't think I noticed that in my testing.

[petsche]

IIRC, this is an issue with how the animations are implemented, and not specific to the GMMK Pro

I can confirm the animation issue I described only affected the merged keyboard-specific config, but I mentioned it here because this PR also touched the same keyboard. I have not personally tested the merged bug fix for the keyboard-specific config, but it did look like it would fix the issue.

[drashna]

Also, please revert the changes to the lib folders. You may need to rebase to fix those.

[glorious-qmk]

Make sure that you run git fetch origin before the rebase, or it may not pick up newer changes.

Done.

[Gigahawk]

Let's go through each of these points one by one

Point 1

Where initialization is done is arbitrary, and extending to support more chips would be done in a similar way.

In any case, it would be trivial to just parameterize the functions in the current implementation.
I didn't bother because it's rather unlikely this chip will be used in another QMK supported board.

Point 2

Again, see #13430, the implementation here is presumably slower by calling spi_write() in a loop instead of using spi_transmit() like in #13430.

To add, the old implementation writing 3 bytes per LED value was already more than fast enough, there was no perceivable lag even using the heavier animation effects.

Point 3

Your interpretation of the timing diagram appears to be incorrect, so let's go through it bit by bit.

POR happens, as the name implies, after Power On/Reset. In our case, this happens when the board is plugged into power, and VDD goes high.
During OTP load, the device is loading overtemperature protection data, and the datasheet recommends that we not pull EN high (it's not clear what would happen, although I suspect the chip would just not do anything until OTP data is loaded).
If you look at the chibios main(), you will see that the bootup process ensures that we wait for far longer than 2ms OTP load time before rgb_matrix_init() is reached*; there is no need to wait inside the initialization code.
Driving the CS lines are also not relevant here.

After EN&&CHIPEN goes high, the chip is immediately ready, as indicated by the Mode becoming Active.
The 100us you indicate is for the OSC signal to be stable.
Because you have not provided the full datasheet, I can only speculate, but OSC seems to be analogous to the SYNC pin from the IS31FL3743. This signal is unused, and as far as I can tell it's not even wired up on the GMMK Pro PCB.

*presumably this happens on other platforms as well, @drashna might be able to confirm

Point 4

By default, all GPIO should be a high impedance input, including CS lines that have yet to be asserted.
When the signal is not being driven, it is assumed that a pullup resistor keeps unused CS lines high, there should be no need to specifically drive all CS pins high prior to using the SPI bus.
It also does not make sense for this driver to have to ensure all CS lines, including ones from other peripherals, to be driven high prior to initialization.

Even if this pullup assumption does not hold true, because we are only doing writes on the SPI bus, there is no concern for contention on the MISO line, it would be perfectly fine to intentionally have all AW20216 CS lines be low during initialization (although implementing this probably wouldn't be worth the effort).

Point 5

Using a delay of 100us is just as arbitrary as assuming a delay of 0us.
Testing indicates that not having any delays in the code works perfectly fine, as evidenced by the existing implementation.

If you look at the datasheet for the IS31FL3743, you will see that the minimum delay is on the order of nanoseconds.
Given that the STM32F303 runs at 72MHz, it is completely plausible that just the code execution time is more than enough delay to not cause problems with the AW20216.

It is highly unlikely that this driver will ever be used for any board other than the GMMK Pro, I think it is fine to just not have any delays until we find out pairing this chip with a faster MCU breaks something.

Point 6

SWSEL is not configured because it offers no perceivable benefit.
The only potential benefit of allowing SWSEL to be configured would be a higher max brightness, since the chip can skip unused SW lines.
I see no reason why we would want this in a keyboard, it would be pretty jarring to have some keys be brighter than others.
A correct implementation of SWSEL per chip would include unique current scaling values per chip as well to compensate for this difference in brightness, but by then you've put in work to do effectively nothing.

There's also little argument for using fewer SW lines from a speed standpoint since the existing code writes the entire PWM buffer every time.
I don't think the potential speed gains are worth implementing specific framebuffer sizes per chip.

Looking at the leaked spreadsheet, it looks like the hardware designers had similar thoughts, given how SW11_1 is unnecessarily used even for this unreleased 104-key variant.
The existing 75% GMMK Pro could have had even more SWXX_1 lines disabled if using the SWSEL feature was a priortity

[drashna]

Please revert. would prefer to keep the ISSI like config here.

[glorious-qmk]

Not sure what you mean. Could you please address me to the ISSI driver to see the style?

[hardpoorcorn]

Any idea on how to progress this?

[tzarc]

Any idea on how to progress this?

develop branch already has a working driver from @Gigahawk. Is there something specific in this PR you need?

[hardpoorcorn]

Any idea on how to progress this?

develop branch already has a working driver from @Gigahawk. Is there something specific in this PR you need?

I apologize for having tracked the wrong PR here. Was trying to understand where RGB support was. Didnt realize it had been handled elsewhere.

[ForsakenRei]

Any idea on how to progress this?

develop branch already has a working driver from @Gigahawk. Is there something specific in this PR you need?

Dumb questions from someone new to QMK: the only thing I need here is switching to devlop branch then I should be able to make a firmware with RGB support? Any plan on when to roll it out to master branch?

[Gigahawk]

Any idea on how to progress this?

develop branch already has a working driver from @Gigahawk. Is there something specific in this PR you need?

A dumb questions from someone new to QMK: the only thing I need here is switching to devlop branch then I should be able to make a firmware with RGB support? Any plan on when to roll it out to master branch?

Yea you can just compile off develop.

https://beta.docs.qmk.fm/developing-qmk/breaking-changes/breaking_changes

[ForsakenRei]

Any idea on how to progress this?

develop branch already has a working driver from @Gigahawk. Is there something specific in this PR you need?

A dumb questions from someone new to QMK: the only thing I need here is switching to devlop branch then I should be able to make a firmware with RGB support? Any plan on when to roll it out to master branch?

Yea you can just compile off develop.

https://beta.docs.qmk.fm/developing-qmk/breaking-changes/breaking_changes

Thanks! I will give it a try.

[fvanc]

Hey folks! Any updates on this?

[tzarc]

Hey folks! Any updates on this?

At this point, you can use the develop branch. Even if this were to be merged, you'd need to use develop anyway.

@glorious-qmk can you explain why this implementation should be taken in preference to the existing one in the repository already?

[pinpox]

Hey folks! Any updates on this?

At this point, you can use the develop branch. Even if this were to be merged, you'd need to use develop anyway.

Is the develop branch stable or will I run any danger of bricking my keyboard? Any reason why it's not merged?

[tzarc]

Hey folks! Any updates on this?

At this point, you can use the develop branch. Even if this were to be merged, you'd need to use develop anyway.

Is the develop branch stable or will I run any danger of bricking my keyboard? Any reason why it's not merged?

I've used the develop branch exclusively for more than 12 months. It's as stable as much as possible, and allows us to keep working on shiny new features whilst not interrupting people who use master as their base -- which is most people.

As for why this isn't merged:

• It has conflicts with the develop branch and is quite literally unmergeable in its current state
• There's a working RGB driver already in develop provided by the community, and there's not been a reason why this PR should be used instead.

[ForsakenRei]

Hey folks! Any updates on this?

At this point, you can use the develop branch. Even if this were to be merged, you'd need to use develop anyway.

Is the develop branch stable or will I run any danger of bricking my keyboard? Any reason why it's not merged?

Based on my own experience, it is pretty stable as long as your code is correct. I have been using the develop branch for a while and keep tinkering my own keymap/firmware then flash it, never bricked my GMMK Pro. With the develop branch you have full control of RGB(need spend a little time if you want fancy light), keymaps, encoder and VIA they all work, which meets my expectation.
IMO since we already have the job done, there is no need to reinvent the wheel. Glorious wants to merge this PR and it is also listed on their develop road map, but at this time point it might be not necessary. Most of the end users didn't really care who did the job, as long as it works.

[pinpox]

Most of the end users didn't really care who did the job, as long as it works.

I indeed don't care, but if @glorious-qmk could state their position on this it would be very beneficial to have "official" approval that this won't void their warrenty or similar. @ForsakenRei Thanks for the info, will give it a try later!

[andrebrait]

Hey folks! Any updates on this?

At this point, you can use the develop branch. Even if this were to be merged, you'd need to use develop anyway.

Is the develop branch stable or will I run any danger of bricking my keyboard? Any reason why it's not merged?

Based on my own experience, it is pretty stable as long as your code is correct. I have been using the develop branch for a while and keep tinkering my own keymap/firmware then flash it, never bricked my GMMK Pro. With the develop branch you have full control of RGB(need spend a little time if you want fancy light), keymaps, encoder and VIA they all work, which meets my expectation.
IMO since we already have the job done, there is no need to reinvent the wheel. Glorious wants to merge this PR and it is also listed on their develop road map, but at this time point it might be not necessary. Most of the end users didn't really care who did the job, as long as it works.

But is that the only difference here? I guess people asked @glorious-qmk a number of times what's different and why should this be merged but they haven't presented a reason for it, like "it works better because of X" or "it is closer to what the specsheet says" or whatever.

Anyway, I think we should merge some of the changes here (possibly as another PR) such as the default keymap and some other things. The default keymap here reflects the stock keymap and stock function layer.

Are all the RGB effects also implemented in the existing driver?

[grimsandwich]

Hey folks! Any updates on this?

At this point, you can use the develop branch. Even if this were to be merged, you'd need to use develop anyway.

Is the develop branch stable or will I run any danger of bricking my keyboard? Any reason why it's not merged?

Based on my own experience, it is pretty stable as long as your code is correct. I have been using the develop branch for a while and keep tinkering my own keymap/firmware then flash it, never bricked my GMMK Pro. With the develop branch you have full control of RGB(need spend a little time if you want fancy light), keymaps, encoder and VIA they all work, which meets my expectation.
IMO since we already have the job done, there is no need to reinvent the wheel. Glorious wants to merge this PR and it is also listed on their develop road map, but at this time point it might be not necessary. Most of the end users didn't really care who did the job, as long as it works.

But is that the only difference here? I guess people asked @glorious-qmk a number of times what's different and why should this be merged but they haven't presented a reason for it, like "it works better because of X" or "it is closer to what the specsheet says" or whatever.

Anyway, I think we should merge some of the changes here (possibly as another PR) such as the default keymap and some other things. The default keymap here reflects the stock keymap and stock function layer.

Are all the RGB effects also implemented in the existing driver?

I just tried compiling the develop branch, I found that all RGB effects to be functional. I haven't tried cloning the glorious's fork, but I don't have any issue with the currently merged implementation in the develop branch. It is now my daily driver and I can remap the rotary encoder. I also managed to make it work with VIA, but VIA doesn't support rotary encoder yet.

[arazabishov]

I've used the develop branch exclusively for more than 12 months. It's as stable as much as possible, and allows us to keep working on shiny new features whilst not interrupting people who use master as their base -- which is most people.

Does online QMK configurator build firmware from the develop branch? If not, how often master is merged with develop?

[tzarc]

I've used the develop branch exclusively for more than 12 months. It's as stable as much as possible, and allows us to keep working on shiny new features whilst not interrupting people who use master as their base -- which is most people.

Does online QMK configurator build firmware from the develop branch? If not, how often master is merged with develop?

As per the current breaking changes timeline, we're on-track for a merge on August 28. We're in the process of setting up infrastructure to provide a mirror of configurator that targets develop, but that's in the pipeline and nowhere near ready for deployment.

[zvecr]

Going to close this due to no movement on the previous statement, there's not been a reason why this PR should be used instead. Some of the changes have already been rolled up, to both keyboard and the community provided driver.

While we would accept improvements to the existing implementation, the follow has to be done as part of any future PR;

• changes should target the current implementation
• reasons for the changes need to be defined upfront and justified