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

Add keyboard Lily58 Koca #24847

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

Conversation

Hardware7253
Copy link

@Hardware7253 Hardware7253 commented Jan 21, 2025

Description

Add keyboard Lily58 Koca to the keyboards folder. This is placed in a seperate folder to the other Lily58's because I am not the original maintainer.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout (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).

@Hardware7253 Hardware7253 marked this pull request as ready for review January 21, 2025 10:18
keyboards/lily58_koca/config.h Outdated Show resolved Hide resolved
keyboards/lily58_koca/config.h Outdated Show resolved Hide resolved
keyboards/lily58_koca/halconf.h Outdated Show resolved Hide resolved
keyboards/lily58_koca/keymaps/default/rules.mk Outdated Show resolved Hide resolved
keyboards/lily58_koca/readme.md Outdated Show resolved Hide resolved
keyboards/lily58_koca/readme.md Outdated Show resolved Hide resolved
Hardware7253 and others added 6 commits January 21, 2025 23:30
Co-authored-by: Ryan <fauxpark@gmail.com>
Co-authored-by: Ryan <fauxpark@gmail.com>
Co-authored-by: Ryan <fauxpark@gmail.com>
Co-authored-by: Ryan <fauxpark@gmail.com>
Co-authored-by: Ryan <fauxpark@gmail.com>
@Hardware7253 Hardware7253 requested a review from fauxpark January 21, 2025 10:45
keyboards/lily58_koca/keyboard.json Outdated Show resolved Hide resolved
Hardware7253 and others added 2 commits January 21, 2025 23:54
@Hardware7253 Hardware7253 requested a review from fauxpark January 21, 2025 10:58
keyboards/lily58_koca/keyboard.json Outdated Show resolved Hide resolved
@filterpaper
Copy link
Contributor

Can you consider not using the lily58 prefix name? Duplicating names of popular boards will oftentimes lead users to an incorrect firmware and generate unnecessary support questions on the QMK Discord server.

Fixed layout so that is compatible with original lily58 keymaps
@Hardware7253 Hardware7253 requested a review from sigprof January 21, 2025 22:42
@Hardware7253
Copy link
Author

Can you consider not using the lily58 prefix name? Duplicating names of popular boards will oftentimes lead users to an incorrect firmware and generate unnecessary support questions on the QMK Discord server.

The name is sort of set in stone having been already printed on circuit boards. Instead I'll compromise and add something to the readme redirecting users of the original Lily58 and it's various versions.

@filterpaper
Copy link
Contributor

Can you consider not using the lily58 prefix name? Duplicating names of popular boards will oftentimes lead users to an incorrect firmware and generate unnecessary support questions on the QMK Discord server.

The name is sort of set in stone having been already printed on circuit boards. Instead I'll compromise and add something to the readme redirecting users of the original Lily58 and it's various versions.

QMK readme is not the first reference point for users unfortunately. A prominent firmware guide about these duplicate names on vendor(s) product pages will be more useful.

Redirect users of original Lily58
@Hardware7253
Copy link
Author

Can you consider not using the lily58 prefix name? Duplicating names of popular boards will oftentimes lead users to an incorrect firmware and generate unnecessary support questions on the QMK Discord server.

The name is sort of set in stone having been already printed on circuit boards. Instead I'll compromise and add something to the readme redirecting users of the original Lily58 and it's various versions.

QMK readme is not the first reference point for users unfortunately. A prominent firmware guide about these duplicate names on vendor(s) product pages will be more useful.

I did add something to readme but I think you're right, users will definitely not read it and wonder why the firmware wont upload. I'll change the directory name to koca. It'll be slightly more confusing for users of my design but save original Lily58 users some trouble.

Avoid confusion for original lily58 users
@filterpaper
Copy link
Contributor

It'll be slightly more confusing for users of my design but save original Lily58 users some trouble.

Such user confusion happens both ways based on support questions in Discord—vendor board users trying to use the open source version and vice versa.

@Hardware7253
Copy link
Author

It'll be slightly more confusing for users of my design but save original Lily58 users some trouble.

Such user confusion happens both ways based on support questions in Discord—vendor board users trying to use the open source version and vice versa.

I've seen this comment sitting for a while. Is there anything else I can do to prevent users from trying to flash the wrong firmware though? I really don't think original Lily58 users will stumble across this firmware in the QMK repo due to it's new name. On my side I can make sure my vendor directs my users to the koca firwmare.

Or are you suggesting that I change the name entirely so that it has nothing to do with the Lily58?

keyboards/koca/board.h Outdated Show resolved Hide resolved
keyboards/koca/config.h Outdated Show resolved Hide resolved
keyboards/koca/halconf.h Outdated Show resolved Hide resolved
keyboards/koca/keyboard.json Outdated Show resolved Hide resolved
Comment on lines 16 to 17
"cols": ["A10", "A9", "A8", "C9", "C8", "C7", "B2", "B13", "B12", "B14", "B15", "C6", "A6", "A7", "C4", "C5", "B0", "B1", "B5", "A0", "A1", "A2", "A3", "A4", "A5", "C14", "C13", "B7", "B6"],
"rows": ["C12"]
Copy link
Member

Choose a reason for hiding this comment

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

This is .... unfortunate. A direct pin matrix would have been better here (and faster), and wouldn't have needed the C12 pin (just ground).

Copy link
Author

Choose a reason for hiding this comment

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

Yeah. I have no idea how this oversight made it so far. I'm keeping it at this point so my v1 PCBs will work.

Copy link
Contributor

Choose a reason for hiding this comment

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

You may be able to declare the matrix as direct pin, and then initialize the C12 pin to output low in matrix_init_kb():

void matrix_init_kb(void) {
    // Activate the only row pin so that the matrix can be handled as direct pins.
    gpio_set_pin_output(C12);
    gpio_write_pin_low(C12);

    matrix_init_user();
}

In the V2 PCB you can just leave the C12 pin unconnected and keep the rest of pin assignments the same, so that the firmware would be compatible with both PCB versions.

Copy link
Author

Choose a reason for hiding this comment

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

Good idea, I'll start testing this now.

Copy link
Author

Choose a reason for hiding this comment

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

You may be able to declare the matrix as direct pin, and then initialize the C12 pin to output low in matrix_init_kb():

void matrix_init_kb(void) {
    // Activate the only row pin so that the matrix can be handled as direct pins.
    gpio_set_pin_output(C12);
    gpio_write_pin_low(C12);

    matrix_init_user();
}

In the V2 PCB you can just leave the C12 pin unconnected and keep the rest of pin assignments the same, so that the firmware would be compatible with both PCB versions.

I ended up switching entirely to a direct matrix and will change the PCB. Users will never get my first PCB, I just ordered 5 for prototyping.

Copy link
Author

Choose a reason for hiding this comment

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

It's been a little while since the switch to a proper direct matrix. Would someone consider reviewing this please?

keyboards/koca/keymaps/default/keymap.c Outdated Show resolved Hide resolved
keyboards/koca/keymaps/default/keymap.c Show resolved Hide resolved
keyboards/koca/mcuconf.h Outdated Show resolved Hide resolved
Hardware7253 and others added 2 commits January 29, 2025 16:30
Co-authored-by: Drashna Jaelre <drashna@live.com>
@Hardware7253 Hardware7253 requested a review from drashna January 29, 2025 03:49
Change supported revs. This won't generate additional questions for old revs because their design files can only be found through the keyboards old git commits.

The current design iteration and all future revs will be wired properly for a direct matrix.
@Hardware7253
Copy link
Author

Would somebody mind reviewing this? I've addressed all previous issues.

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

Successfully merging this pull request may close these issues.

5 participants