qmk_firmware icon indicating copy to clipboard operation
qmk_firmware copied to clipboard

Addition of the Ymdk Oni Tsangan/HHKB-like pcb

Open ChrisGVE opened this issue 10 months ago • 11 comments

This is the addition of all LED PCBs with per-key RGB and underflow in the same driver. The board had already been submitted in the past (#21004 ) but wasn't accepted at the time due to uncertainty related to its naming. I've linked the official website, and upon receiving the board, before reflashing it with my firmware, the board was recognized as ONI_HHKB on VIA desktop.

Apologies for the mess I made since this is the third time I submit this PR. I could not revert to the first PR since in the meantime I had renamed my branch, and I was prevented from re-opening it. This is a new lesson for me, but I am sorry for the confusion it creates.

I have however included all comments in this PR, including running qmk format-json -i keyboard.json, but as always I am happy to make any further changes, as required.

Description

Types of Changes

  • [ ] Core
  • [ ] Bugfix
  • [ ] New feature
  • [ ] Enhancement/optimization
  • [x] Keyboard (addition or update)
  • [ ] Keymap/layout (addition or update)
  • [ ] Documentation

Issues Fixed or Closed by This PR

Checklist

  • [x] My code follows the code style of this project: C, Python
  • [x] 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.
  • [x] I have read the CONTRIBUTING document.
  • [ ] I have added tests to cover my changes.
  • [x] I have tested the changes and verified that they work and don't break anything (as well as I can manage).

ChrisGVE avatar May 03 '25 12:05 ChrisGVE

I have run a qmk git-submodule before submitting but I see that there is still a problem with the build. Is there something else I could try to make this work.

ChrisGVE avatar May 03 '25 12:05 ChrisGVE

Note: The CI is reporting submodule changes, but these are environmental differences. I haven't intentionally modified any submodules - only the keyboard-specific files.

I have checked everything I could think of with git CLI (and with an AI agent), and I cannot find the culprit.

ChrisGVE avatar May 03 '25 13:05 ChrisGVE

Assuming qmk/qmk_firmware is set as your upstream and chrisgve/qmk_firmware is your origin, please try running the following commands:

  1. cd qmk_firmware
  2. git fetch upstream master
  3. git checkout upstream/master
  4. qmk doctor. Verify that you are now working with version 0.27 of the repo. You will be prompted to clone the submodules; answer YES.
  5. git checkout ymdk_oni
  6. git add lib
  7. git commit -m "revert submodule changes"

Before pushing to origin again, make sure everything still builds without the submodule modifications.

Always run git status before committing to verify you are not checking in unintended changes.

lesshonor avatar May 03 '25 16:05 lesshonor

Please do not close and open new PRs. Especially when previously made suggestions have not been addressed, like this, which is still an issue in this PR, despite being marked as resolved in the previous one?

Any future PRs for this keyboard are going to be closed. If you continue to have issues with submodules, ask for help in the QMK Discord.

Understood, and again, I'm very sorry for the mess. I've learned my lesson. In the meantime, I am trying the @lesshonor solution to get back on track.

ChrisGVE avatar May 03 '25 17:05 ChrisGVE

Here are my updates on the work done so far

  • I have applied the changes that had been requested in #25158
  • Thanks to @lesshonor, I was able to resolve the library issue
  • I've applied @dunk2k request for changes
  • I've added an alias for the LAYOUT_60_tsangan_hhkb
  • I've applied qmk format-json -i keyboard.json to ensure proper formatting
  • I've run qmk lint -kb ymkd/oni_hhkb
  • All checks, including CI, are now successful Is there anything else I should do?

@zvecr is the PR still invalid in its present condition?

ChrisGVE avatar May 06 '25 19:05 ChrisGVE

Is there anything I missed preventing the merge?

ChrisGVE avatar May 28 '25 10:05 ChrisGVE

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[bot] avatar Jul 13 '25 02:07 github-actions[bot]

Typing this from a keyboard running this pull request :)

Do you know what is still required to merge this PR?

ChrisGVE avatar Jul 16 '25 10:07 ChrisGVE

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[bot] avatar Sep 05 '25 02:09 github-actions[bot]

I know that the admins are underwater, but this PR has been open for months. What do we need to have this pretty straightforward PR approved and merged into the main branch?

ChrisGVE avatar Sep 06 '25 11:09 ChrisGVE

@zvecr, @waffle87, @dunk2k, I just wanted to ask you a favor to approve this pending PR and merge it. That would be really great! Thanks

ChrisGVE avatar Oct 05 '25 17:10 ChrisGVE