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

Massdrop requirements before next board gets merged #11119

Open
tzarc opened this issue Dec 4, 2020 · 3 comments
Open

Massdrop requirements before next board gets merged #11119

tzarc opened this issue Dec 4, 2020 · 3 comments

Comments

@tzarc
Copy link
Member

tzarc commented Dec 4, 2020

This issue tracks the items required for QMK to merge any new Massdrop keyboards. The upstream protocol ("arm_atsam") is not in a state that the QMK collaborators are comfortable with, and as such have decided to decline future PRs targeting arm_atsam until the following cleanup occurs:

  • Merge mdloader#16 -- this is a significant usability improvement for customers, and would prevent us having to explain why QMK doesn't persist data on Massdrop boards after poweroff -- at this point our response tends to be "QMK would work if Massdrop would support it"
  • Move the actual Atmel/Microchip provided code into a lib/atsam or equivalent. The directory tmk_core/protocol/arm_atsam should be limited to the QMK-specific MCU+USB initialisation code as well as the main function, and thus is only inclusive of whatever is required to expose the vendor-supplied APIs to provide a concrete implementation of the interface expected by QMK.
  • Move the non-Atmel/Microchip keyboard-specific code from the arm_atsam protocol (i.e. the usb2422, led drivers etc.) into to their respective keyboards, as these aren't guaranteed to be provided for every single SAMD51 board ever to be designed.
  • Break out an I2C driver as per the i2c_master interface that the rest of QMK uses into drivers/arm_atsam, converting across the code that currently addresses things directly.

The i2c_master change will then enable the Massdrop boards to have the following transformations occur:

  • Convert each board's LED matrix code to use QMK's rgbmatrix subsystem -- you're free to keep a "legacy mode" using your original implementation, but that should be moved to the keyboard directory and legacy mode should be opt-in, not opt-out. There is an ISSI3733 driver already in QMK.
  • Convert any of the other I2C code in use (e.g. usb2422 initialisation) to i2c_master.

At this point we've raised qmk_firmware#10904 to warn users of Massdrop's lack of support -- if we gain some traction on the above issues this will no longer be necessary (or get reverted, if it subsequently merged).

Please try to minimise each change to a single subset of modifications -- historically Massdrop's PRs have included many unrelated changes and nobody decides to review. It'd be great if this could be avoided in future. It would also be preferable if there was an active contributor from Massdrop's side, as most of the PRs received seem to be abandonware once lodged -- any requested changes seem to be ignored.

@tzarc
Copy link
Member Author

tzarc commented Dec 21, 2020

Alternatively, feel free discuss with us the addition of RIOT-OS as a HAL instead of arm_atsam. Or anything else that has upstream support.

@tzarc tzarc added the on hold label Mar 3, 2021
rrevans added a commit to rrevans/qmk_firmware that referenced this issue Jul 6, 2021
Notably this replaces the custom debounce with the common debounce API
which in turn resolves key double-pressing issues.

The original debounce sets a timer when any key changes then stops all
scanning until the timer expires. Since it is not scanning, the code
cannot suppress long bounces or multiple keys bouncing at once.

As a result, those events cause unwanted double presses.

This fix could have patched the buggy debounce or integrated the
debounce API, but converting to the lite API is simpler and reduces the
amount of custom code to maintain.

In theory this board doesn't strictly need any custom matrix code at all
except that this platform lacks proper library support (see qmk#11119).
@zvecr zvecr self-assigned this Sep 30, 2021
@mmirate
Copy link

mmirate commented Nov 26, 2021

Merge mdloader#16 ...

Did the merging of Massdrop/mdloader#62 suffice?

@drashna
Copy link
Member

drashna commented Nov 26, 2021

Merge mdloader#16 ...

Did the merging of Massdrop/mdloader#62 suffice?

No. That's only one of several part that are needed. the mdloader was more of a "bare minimum".
The rest of the listed parts still need work done. However, a lot of prep work has been done to get everything in a place where the massdrop bards can be better supported

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

4 participants