qmk_firmware icon indicating copy to clipboard operation
qmk_firmware copied to clipboard

Published 20 hours ago • verified publisher icon qmk

Added Keychron C3 Pro

Open KeychronMacro opened this issue 1 year ago • 8 comments

Added Keychron C3 Pro and request for a review, thanks!

Description

  • Added keychron/c3_pro/ansi/red.

Types of Changes

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

KeychronMacro avatar Aug 29 '23 03:08 KeychronMacro

@fauxpark hi, do you have time to check this? thanks

keychron-dev avatar Nov 08 '23 14:11 keychron-dev

Hey, when merging this branch with current qmk/master in my machine I got some issues: error: 'SNLED27351_I2C_ADDRESS_1' undeclared (gist), probably caused by the DRIVER_ADDRESS_n defines changes that were merged in this commit https://github.com/qmk/qmk_firmware/commit/d56ee70c524b4fc4d1638e5e8c4bdeb89e752993

I got it compiling and running in my keychron c3 pro by applying those renames on top of this MR: https://github.com/emmamm05/qmk_firmware/commit/528c0e4d76603702abd837bdd9272f9db2883109

emmamm05 avatar Dec 10 '23 18:12 emmamm05

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 Feb 07 '24 01:02 github-actions[bot]

can we have someone review this pull? thanks

keychron-dev avatar Feb 07 '24 01:02 keychron-dev

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 Jun 17 '24 01:06 github-actions[bot]

posting toplevel since comment is resolved

This file can be deleted.

it looks like make keychron/c3_pro/ansi/red:default as mentioned in readme does not work with this file missing, and qmk list-keyboards, which make depends on for building valid targets, ignores any paths with /keymaps/ in it.

maybe the readme just needs a change or the file is needed?

I'm new to this project, apologies if any statements are uninformed - I could just be completely wrong :)

brad-alexander avatar Sep 07 '24 01:09 brad-alexander

@brad-alexander If you are testing this PR locally, your repo is out of date - there is a keyboard.json in that folder which should serve the same purpose (marking it as a buildable keyboard) as rules.mk previously did. That's why the other comment in my review above was to rename the info.json.

fauxpark avatar Sep 07 '24 02:09 fauxpark

So after compiling this for myself and figuring out how qmk works and what is expected of a PR for this repo this seems like a good PR to merge.

Notupus avatar Sep 11 '24 11:09 Notupus