[Keyboard] Add keyboard Moon #5976
Conversation
There was a problem hiding this comment.
Because of the Community Layouts feature, enabled here: https://github.com/qmk/qmk_firmware/blob/afce7e155420c77cb1ebd0101348a533f582108e/keyboards/moon/rules.mk#L55... the ISO macros and keymaps you have in this PR need to be updated. The Community Layouts feature only works if all the supporting keyboards expect the keycodes to be sent in the same order, and your codebase here deviates from that pattern. Your info.json files already have both ISO layouts' objects in the correct order.
Additionally, the info.json file should also have another JSON object to correspond with the LAYOUT_all macro.
|
@zvecr can you review please. This is your area of expertise. |
| @@ -0,0 +1,78 @@ | |||
| /* Copyright 2019 | |||
There was a problem hiding this comment.
Who is this file actually licensed to/by/for?
There was a problem hiding this comment.
I have no idea. I found it in keyboards/xd84. Though I made a few adjustments to it for things that I believe are bugs.
There was a problem hiding this comment.
Ah, okay. Then a better question, that was not caught would be at @zvecr
There was a problem hiding this comment.
I was previously told that if i did not want personal attribution, to just leave it at Copyright 2019 without my name, however if there has been a shift on this stance then i am happy to update, ie to something QMK releated Copyright 2019 QMK.
As for the bugs, i diffed the files and can only see the updated comments, is this correct? (these match the data sheet, so i can pop them in my local branch, moving pca9555 support to a common location)
There was a problem hiding this comment.
Ah, okay. I think it needs to be more explicit, to be honest, so it's not ... confusing, like this.
@jackhumbert @skullydazed your opinions?
There was a problem hiding this comment.
I'm sorry, looking at it now the file I copied came from keyboards/xd96. The file from keyboards/xd84 doesn't have a copyright notice.
Though what I called bugs are precent in both. Maybe a bug is a bit of an overstatement, and if so I'm sorry.
What I'm talking about is the usage of the command enums.
In the original code pca9555_set_config uses the CMD_OUTPUT_x values and pca9555_set_output uses the CMD_CONFIG_x values.
I'm a bit out of my comfort zone here, but what I get from reading the datasheet and just going by the names of the functions I decided to swap the usage so that pca9555_set_config uses the CMD_CONFIG_x values and vice versa.
Also seeing how the pca9555 now is in use by at least 3 boards. Maybe this could be moved to the drivers in some fashion? Just a thought though as I don't know what the consequences and long term plan regarding this.
There was a problem hiding this comment.
Good catch on the use of the enum values, im unsure how its working so i will have to find out and correct the use elsewhere.
Also seeing how the pca9555 now is in use by at least 3 boards. Maybe this could be moved to the drivers in some fashion
That is what i have locally, and eluded to when i mentioned "moving pca9555 support to a common location" 😄. Given it can take a while for core changes, my preference would be this PR ships its copy of pca9555.[ch] and i refactor it in addition to the xd84 and xd96. That way it doesn't matter how long it takes to get reviews from the QMK members/collaborators .
There was a problem hiding this comment.
That is what i have locally, and eluded to when i mentioned "moving pca9555 support to a common location" 😄.
I see, I didn't catch that. I guess great minds think alike 😸
Given it can take a while for core changes, my preference would be this PR ships its copy of pca9555.[ch] and i refactor it in addition to the xd84 and xd96. That way it doesn't matter how long it takes to get reviews from the QMK members/collaborators .
This sounds like a good approach.
Description
Add support for the Moon keyboard.
I tried my best to do things to an acceptable standard, but it wasn't always clear what is the most up to date and preferred approach.
I have formatted the files using clang-format.
But the provided
.clang-formatspecifiesIndentWidth: '2'which appears to be in conflict with what the coding-conventions states.So I'm not sure what to do about that.
Types of Changes
Checklist