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

Mini M keyboard support with the MiniRazz controller #20106

Closed
wants to merge 26 commits into from

Conversation

purdeaandrei
Copy link
Contributor

Description

Adds support for the Unicomp Mini M keyboard, running the open hardware MiniRazz controller.

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

@purdeaandrei
Copy link
Contributor Author

purdeaandrei commented Mar 12, 2023

Note that I also added #20104 which gets rid of the lint errors of this PR.

EDIT: this is not longer a dependency, cause a workaround has been added.

keyboards/unicomp/mini_m/keymaps/default/keymap.c Outdated Show resolved Hide resolved
keyboards/unicomp/mini_m/keymaps/default/readme.md Outdated Show resolved Hide resolved
keyboards/unicomp/mini_m/minirazz/config.h Outdated Show resolved Hide resolved
keyboards/unicomp/mini_m/info.json Outdated Show resolved Hide resolved
keyboards/unicomp/mini_m/minirazz/minirazz.c Outdated Show resolved Hide resolved
keyboards/unicomp/mini_m/minirazz/readme.md Outdated Show resolved Hide resolved
keyboards/unicomp/mini_m/minirazz/rules.mk Outdated Show resolved Hide resolved
keyboards/unicomp/mini_m/minirazz/rules.mk Outdated Show resolved Hide resolved
@purdeaandrei
Copy link
Contributor Author

@waffle87 thanks for the review! I will work on fixing these a little later, probably in the weekend.

According to marfrit's testing: (Thank You!)
MATRIX_IO_DELAY =  1 results in space spamming the letter "M"
MATRIX_IO_DELAY =  2 only emits one m on short press instead of 5
MATRIX_IO_DELAY =  3 seems to mitigate that completely

For safety I'm increasing this to 4.
purdeaandrei and others added 6 commits March 26, 2023 02:09
Co-authored-by: jack <0x6a73@protonmail.com>
Co-authored-by: jack <0x6a73@protonmail.com>
Co-authored-by: jack <0x6a73@protonmail.com>
Co-authored-by: jack <0x6a73@protonmail.com>
Following code review by waffle87
@purdeaandrei
Copy link
Contributor Author

@waffle87 Thank you, changes you requested have been made, ready for re-review.

return true;
}

bool led_update_user(led_t led_state) {
Copy link
Member

Choose a reason for hiding this comment

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

This should be moved to the keyboard's c file, or moved to a non-default keymap. The default keymap should be kept plain and simple. (same with process_record_user)

Copy link
Contributor Author

@purdeaandrei purdeaandrei Apr 1, 2023

Choose a reason for hiding this comment

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

Of course it won't work with the configurator this way :(

The Mini M, like the IBM SSK has two very inter-related features here:

  1. The Numlock locklight being active activates the embedded numpad in the alpha block (so JKL=123), and those keys have secondary legends.
  2. The Scroll Lock key also has a NumLock secondary legend, and pressing Shift-ScrollLock produces NumLock.

I'd like to implement this default behaviour as the default keymap, such as that clicking Compile on configurator without modifications would produce this behaviour, but from guidelines it sounds like this would be frowned upon?

It would involve moving led_update_user() to the keyboard (making layer 1 special for all keymaps, i.e. activate by numlock being on).
process_record_user() would also have to be moved, so the custom keycode would be defined keyboard-side. And in configurator the custom keycode would show up with the Any key, so Any(QK_USER_0), cause I suppose configurator wouldn't know my custom keycode names.

Is this something I can do, (which I'd prefer to do if it would be accepted), or should I just move this functionality in a keymap called embedded_numpad, and strip it out of the default keymap?

Copy link
Contributor

@lesshonor lesshonor Apr 1, 2023

Choose a reason for hiding this comment

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

[ideally] clicking Compile on configurator without modifications would produce this behaviour

Then you would have to move it to the keyboard level. The default keymap is what gets copied to the newly-created folder when you run qmk new-keymap; Configurator has no knowledge of it.

Configurator generates a completely new JSON keymap based on the keycodes assigned in the GUI. For additional information about Configurator keymaps, including how to use custom keycodes and caveats/restrictions: https://docs.qmk.fm/#/configurator_default_keymaps?id=adding-default-keymaps

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay I have moved it to the keyboard.c file, please review.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Also I moved the layout macro into info.json)

keyboards/unicomp/mini_m/minirazz/matrix.c Outdated Show resolved Hide resolved
keyboards/unicomp/mini_m/minirazz/matrix.c Outdated Show resolved Hide resolved
keyboards/unicomp/mini_m/minirazz/matrix.c Outdated Show resolved Hide resolved
keyboards/unicomp/mini_m/minirazz/matrix.c Outdated Show resolved Hide resolved
keyboards/unicomp/mini_m/minirazz/matrix.c Outdated Show resolved Hide resolved
keyboards/unicomp/mini_m/minirazz/matrix.c Outdated Show resolved Hide resolved
@sigprof
Copy link
Contributor

sigprof commented Apr 1, 2023

The lint failure that you have here looks like a bug in lib/python/qmk/info.py:_check_matrix() — that check assumes that if you have matrix_pins in info.json, there are some actual pins defined there; however, in your case you have only some settings under matrix_pins, but the actual matrix implementation is custom. (If you did not have those settings at all, lint would pass, but you definitely need matrix_pins.ghost for your hardware, and even if you set it in config.h, it would probably be mapped back to JSON anyway before that check.)

You may be able to work around the bug by providing values for matrix_pins.rows and matrix_pins.cols, even if your matrix implementation actually does not use them (use unquoted null for shift register pins — null in JSON would be mapped to NO_PIN in C).

@purdeaandrei
Copy link
Contributor Author

looks like a bug in lib/python/qmk/info.py

I do Have a PR to fix that here: #20104
Thanks, I may do what you said, to not keep the dependency between the PRs...

This is a temporary change until PR 20104 is merged (or some other solution)
@purdeaandrei
Copy link
Contributor Author

You may be able to work around the bug by providing values for matrix_pins.rows and matrix_pins.cols, even if your matrix implementation actually does not use them (use unquoted null for shift register pins — null in JSON would be mapped to NO_PIN in C).

Thanks, I added your suggestion to the inner info.json. I used null for all pins, cause having some of them specified, yet not used would be just confusing.

keyboards/unicomp/mini_m/minirazz/matrix.c Outdated Show resolved Hide resolved
keyboards/unicomp/mini_m/minirazz/matrix.c Outdated Show resolved Hide resolved
keyboards/unicomp/mini_m/minirazz/matrix.c Outdated Show resolved Hide resolved
keyboards/unicomp/mini_m/minirazz/matrix.c Show resolved Hide resolved
keyboards/unicomp/mini_m/minirazz/matrix.c Show resolved Hide resolved
keyboards/unicomp/mini_m/minirazz/matrix.c Outdated Show resolved Hide resolved
@purdeaandrei
Copy link
Contributor Author

purdeaandrei commented Apr 16, 2023

I have added support for some community layouts. Some incompatible user keymaps will likely fail on CI because of that, on discord I've been told that this would not be a problem.
Let me know if there's anything else I need to do here.

"usb": {
"device_version": "0.0.1",
"pid": "0x27DB",
"vid": "0x16C0"
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This vendor ID is provided for free as shared usb id by v-usb:
https://github.com/obdev/v-usb/blob/master/usbdrv/USB-IDs-for-free.txt

Copy link
Contributor

Choose a reason for hiding this comment

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

This is still an issue as it is not a unique VID/PID combo, and if you or community users wish to use VIA or similar could be considered a blocker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not planning on supporting VIA ever.
I am supporting VIAL however, but that doesn't require a unique VID/PID. VIAL detects the keyboard by a magic number embedded somewhere in the SERIAL_NUMBER. (Which for VIAL it means that I have to have a serial number like "purdea.ro:minirazz:MAGIC_NUMBER")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, i don't mean to annoying by refusing to change something, just let me explain my thinking of why I'm resisting this:

If anyone else from my users isn't satisfied with VIAL, and wants to add
VIA support they can of course get a different, unique pair of VID:PID in the future, I'm not gonna block that.

I don't see why I should bother to go requesting a PID code from https://pid.codes/ now, since I don't need it, and occupying a valuable PID code for no reason is wasteful. I am not a keyboard manufacturer, just an open source contributor, I can't afford buying a USB VID, so I must rely on one of the free offerings available.

XAP Also doesn't need Unique VID:PID so hopefully need for unique vid:pid will disappear even for VIA users in the future, so hopefully my hypothetical future users who will want to add VIA support will not need to acquire a vid:pid pair after all.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure that using the 16C0:27DB USB ID for a QMK-based device is proper, because QMK can present more than just a HID keyboard device to the host:

  • HID mouse or joystick — these have separate entries in the ID table;
  • HID digitizer (absolute pointer) — this one would probably be considered a “generic HID class device”;
  • HID interfaces used by console, VIA, Vial or XAP — probably “generic HID class device” too;
  • MIDI — has a separate entry in the ID table, and uses a completely different driver on Windows;
  • CDC-ACM serial — also has a separate entry in the ID table, and uses a completely different driver on Windows.

These non-HID options are probably the most problematic, because apparently Windows caches the driver configuration by idVendor:idProduct:bcdDevice, and can end up using a completely wrong driver if the cached configuration does not match the actual interfaces presented by the device.

Also apparently there are already some users of 16C0:27DB in the repo:

  • dm9records/plaid
  • dm9records/tartan
  • ibm/model_m_4th_gen
  • ibm/model_m_4th_gen/overnumpad_1xb
  • keyhive/lattice60
  • ortho5by12
  • silverbullet44
  • unicomp/classic_ultracl_post_2013
  • unicomp/classic_ultracl_pre_2013
  • unicomp/pc122
  • unicomp/spacesaver_m_post_2013
  • unicomp/spacesaver_m_pre_2013

Copy link
Contributor Author

@purdeaandrei purdeaandrei Apr 22, 2023

Choose a reason for hiding this comment

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

The unicomp/* and ibm/* are mine. I can make changes to them if it becomes necessary. The others are not mine.

dm9records/tartan with 16c0:27db actually has via support: the-via/keyboards#526
This isn't really correct, since it's a shared vid:pid, and I don't think there's a way for VIA to differentiate. dm9records/plaid has a via keymap, but no matching VIA json in the the-via/keyboards repository.

When it comes to compliance with the V-USB terms, I think using 16c0/27db is okay: Under the section "IDs FOR DISCRIMINATION BY SERIAL NUMBER", it says "The USB device MUST provide a textual representation of the serial number, unless ONLY the operating system's default class driver is used.", this implies that both vendor class and an OS default class can coexist, as long as the serial number is specified. It is indeed true that combinations of different kinds of HID is not explicitly permitted, but based on the above I don't think they intended to disallow it. They definitely don't use the precise language of 'usage'/'usage page' that HID documentation uses, instead the say 'Description of use'. I read this as a much fuzzier specification, and even though the keyboard can be compiled to act as a USB mouse too, it still fundamentally is a keyboard, so I think the right VID:PID for it is 16c0/27db, and I think using it this way does comply with the V-USB terms. I fired off a message to Objective Development to check if my interpretation is correct, I will report back if/when I get a response.

Regarding windows caching behavior, I am not really able to test this since I don't have any windows systems. I maybe be able to do some directed testing by borrowing a laptop from someone else. Do you have a reference to where I can read more about this behavior, to figure out how I can test for it? It seems to me like this behavior as it has been described here so far, would affect keymaps too, enabling/disabling certain features, and would affect temporary builds too that enable the CONSOLE for debugging, so I'm surprised that I haven't heard about this. Do windows users really have to overwrite DEVICE_VER in custom keymaps, and when changing what features are enabled?

Copy link
Contributor Author

@purdeaandrei purdeaandrei Apr 24, 2023

Choose a reason for hiding this comment

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

@sigprof I have tested the following, in this order, on a borrowed windows 10 machine:

  1. 16c0:27db with all features enabled (8 interfaces in use), shows up nicely, this is the view in Device Manager, if you select View > Devices By Connection:
    image
    Ableton detects the midi interface nicely.
    This vid:pid has never been plugged into this machine before, so should be starting with a fresh "cache".

  2. 16c0:27db disabled all of the features, it doesn't show up as a composite device anymore. Ableton doesn't detect the MIDI interface anymore:
    image

  3. c0ff:fffe disabled all of the features (should be starting fresh with no cache)
    Same as point 2 above

  4. c0ff:fffe with all features enabled
    Same as point 1 above.
    Ableton correctly detects the midi interface

I don't see any evidence of caching causing any kind of trouble.
If you think caching really is a problem, then could you give me some set of steps that would reproduce it, or at least some documentation that I can reference to come up with a possible theory as to how it could be causing trouble?

…the serial number without duplicating the prefix.

Example keymap config.h:
@github-actions
Copy link

github-actions bot commented Jun 9, 2023

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 bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

@github-actions github-actions bot added the stale Issues or pull requests that have become inactive without resolution. label Jun 9, 2023
@purdeaandrei
Copy link
Contributor Author

not stale, still figuring out what to do about USB ids

@github-actions github-actions bot removed the stale Issues or pull requests that have become inactive without resolution. label Jun 19, 2023
@github-actions
Copy link

github-actions bot commented Aug 4, 2023

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 bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

@github-actions github-actions bot added the stale Issues or pull requests that have become inactive without resolution. label Aug 4, 2023
@purdeaandrei
Copy link
Contributor Author

not stale, still figuring out what to do about USB ids

@github-actions github-actions bot removed the stale Issues or pull requests that have become inactive without resolution. label Aug 5, 2023
@github-actions
Copy link

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 bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

@github-actions github-actions bot added the stale Issues or pull requests that have become inactive without resolution. label Sep 20, 2023
Copy link
Member

Choose a reason for hiding this comment

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

Most of this (eg everything but the src part) can be moved to info.json, and should be.

qmk info -f ascii or qmk migrate can help with that.

# Build Options
# change yes to no to disable
#
BOOTMAGIC_ENABLE = yes
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

Also, SOLENOID should be renamed to solenoid

@drashna drashna removed the stale Issues or pull requests that have become inactive without resolution. label Sep 23, 2023
Copy link

github-actions bot commented Nov 8, 2023

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 bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

@github-actions github-actions bot added the stale Issues or pull requests that have become inactive without resolution. label Nov 8, 2023
Copy link

github-actions bot commented Dec 9, 2023

Thank you for your contribution!
This pull request has been automatically closed because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, or re-open when it's ready.
// [stale-action-closed]

@github-actions github-actions bot closed this Dec 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keyboard keymap stale Issues or pull requests that have become inactive without resolution.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants