melody96: Added layout for non-hotswap PCB

[tpstevens]

Description

According to YMDK (see Manuals section), using the hotswap layout on the non-hotswap PCB requires swapping the position of ENTER and |. Many users aren't aware of this:

• Reddit post 9 months ago
• Reddit post 6 months ago
• Github issue open since 2019
• Github issue open since 12 days ago

To fix it, I created a new layout macro for the default ANSI hotswap keymap that supports the non-hotswap PCB.

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

• Enter Key and \| key seem to be swapped in the matrix. Melody96 #6471
• [Bug] melody96 wrong keymap layout #12666

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

[fauxpark]

I think this layout macro needs a more accurate name, as the non-hotswap board supports more than one layout (hence why the hotswap version gets its own macro). It appears to be the same as the existing LAYOUT_std60_split_num0 but with two extra keys (the split right shift and 5 keys to the right of space vs 4).

[tpstevens]

Fair enough. How about LAYOUT_non_hotswap_c to match YMDK's documentation? https://ymdkey.com/products/kit-1-aluminum-cnc-top-acrylic-cnc-bottom-ymdk-melody-96-hotswap-pcb-keyboard-kit

[fauxpark]

Since there is only one possible hotswap layout, all others can be assumed to be for the soldered version, so there is no need to mention it at all. This may be a little outside the scope of this PR, but I'd say the current layouts (plus this new one) should be named as such:

LAYOUT_all
LAYOUT_hotswap
LAYOUT_ansi_split_num0
LAYOUT_ansi_split_rshift_num0

And of course this list is not exhaustive as the board can support many more combinations of layout options, as shown by the diagram below the Layout A/B/C/D images.

[tpstevens]

That's a good point about removing "hotswap" from the name. I double-checked the configurator, and the default LAYOUT layout does work with the non-hotswap PCB with most of the same key positions I proposed in this PR. The only real difference is that I have a KC_NO in the key to the right of the split left shift.

I think that users reported the issues above because they weren't sure which parameters in the LAYOUT macro corresponded to which keys (e.g. if I have the full-size left-shift instead of the split shift, does KC_LSFT go in the far left key or one to the right?) and thought that LAYOUT_hotswap would be a convenient way to get that layout for their non-hotswap PCB. That's how I ran into the issue, at least.

With that in mind, I think that ideally we should document in the readme (which appears in the Configurator) which keys in the LAYOUT macro correspond to which configurations in YMDK's non-hotswap diagram. That way we don't need to create new macros for new layouts. However, we've already added one macro for LAYOUT_std60_split_num0 - do you think it's worth allowing contributors to add more? Adding better documentation to the default LAYOUT already makes it easier to understand, but I think it would be nice to keep LAYOUT_std60_split_num0 and get more presets.

I like the naming convention you propose, but I would suggest using YMDK's A, B, C, and D where possible:

LAYOUT_all (currently called LAYOUT)
LAYOUT_ansi_split_num0 (currently called LAYOUT_std60_split_num0)
LAYOUT_c (what I proposed in this PR)
LAYOUT_hotswap_c (currently called LAYOUT_hotswap)

Adding the rest of YMDK's layouts in both hotswap and non-hotswap forms would also be nice. I'm happy to do the work - I created my own hotswap layout C with mill-max sockets and can test all the combinations pretty easily.

[fauxpark]

The YMDK names could be aliases of more specific layout names (the ones I proposed). See for example the Whitefox's layout macros:
https://github.com/qmk/qmk_firmware/blob/master/keyboards/whitefox/whitefox.h
You can see we prefer a more "standardised" naming convention. This is to help with the Community Layouts feature, where all boards that support eg. an ANSI TKL layout would have a layout macro named LAYOUT_tkl_ansi and one can simply write a keymap using that layout and compile+flash it on any of said boards.

I also realised that although it is not set up, the Melody96 would support the 96_ansi layout - it is exactly the same as LAYOUT_hotswap - as well as 96_iso. Since this new non-hotswap layout is the same but swaps two matrix positions, I think this problem should be solved by creating a new subfolder specifically for the hotswap board, which contains only a single LAYOUT_96_ansi layout.

[tpstevens]

I was unaware of Community Layouts; that's a neat idea. So ideally we would have a new hotswap folder with 96_iso, 96_ansi, and two other layouts to cover all 4 options?

[fauxpark]

As far as I'm aware there would only be one layout for the hotswap folder, unless they are making multiple hotswap boards for each layout.

[tpstevens]

I don't know if they actually use multiple PCB variants for each hotswap type, but I do know that the buyer can only choose one for their board and can't switch to another layout. That's why I figured we should include multiple layouts (to avoid the same user confusion for the LAYOUT macro as far as which keycode goes where).

[tpstevens]

@fauxpark I'm having a little trouble with moving the hotswap variant of the PCB to its own folder. I created a hotswap folder with a new info.json, rules.mk, and hotswap.h/.c with LAYOUT_all/LAYOUT_b/LAYOUT_96_ansi. I then created a new 96_ansi keymap and moved the existing hotswap zunger keymap to hotswap/keymaps. Both keymaps fail to compile (e.g. qmk compile -kb melody96/hotswap -km 96_ansi) -- QMK isn't seeing the new layout macros.

Am I messing something up with the folder structure or include paths? My second commit (which I'll revert) has the changes.

I've seen a few different ways to do it. The clueboard folder has several subfolders, each with a compilable keyboard inside. 1upkeyboards/sweet16 has a top-level compilable keyboard with a sweet16.h/.cpp and subfolders for each revision, each with their own header. What's the correct approach for melody96?

[drashna]

Could you add a GPL2+ compatible license header here?

For instance:

qmk_firmware/quantum/template/base/keyboard.c

Lines 1 to 15 in 28ff511

	/* Copyright %YEAR% %YOUR_NAME%
	*
	* This program is free software: you can redistribute it and/or modify
	* it under the terms of the GNU General Public License as published by
	* the Free Software Foundation, either version 2 of the License, or
	* (at your option) any later version.
	*
	* This program is distributed in the hope that it will be useful,
	* but WITHOUT ANY WARRANTY; without even the implied warranty of
	* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
	* GNU General Public License for more details.
	*
	* You should have received a copy of the GNU General Public License
	* along with this program. If not, see <http://www.gnu.org/licenses/>.
	*/

[drashna]

Could you add a GPL2+ compatible license header here?

For instance:

qmk_firmware/quantum/template/base/keyboard.c

Lines 1 to 15 in 28ff511

	/* Copyright %YEAR% %YOUR_NAME%
	*
	* This program is free software: you can redistribute it and/or modify
	* it under the terms of the GNU General Public License as published by
	* the Free Software Foundation, either version 2 of the License, or
	* (at your option) any later version.
	*
	* This program is distributed in the hope that it will be useful,
	* but WITHOUT ANY WARRANTY; without even the implied warranty of
	* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
	* GNU General Public License for more details.
	*
	* You should have received a copy of the GNU General Public License
	* along with this program. If not, see <http://www.gnu.org/licenses/>.
	*/

[drashna]

Suggested change

	#define LAYOUT LAYOUT_all
	#define LAYOUT_c LAYOUT_96_ansi

[tpstevens]

Thanks for the feedback -- I'll add the license headers. What's the rationale for removing the LAYOUT aliases? I was following the style of whitefox based on fauxpark's suggestions.

This is still a work in progress, as I need a little bit of guidance as far as the compilation errors I'm having with the hotswap layout. Not sure of the folder structure yet.

[drashna]

Could you add a GPL2+ compatible license header here?

For instance:

qmk_firmware/quantum/template/base/keyboard.c

Lines 1 to 15 in 28ff511

	/* Copyright %YEAR% %YOUR_NAME%
	*
	* This program is free software: you can redistribute it and/or modify
	* it under the terms of the GNU General Public License as published by
	* the Free Software Foundation, either version 2 of the License, or
	* (at your option) any later version.
	*
	* This program is distributed in the hope that it will be useful,
	* but WITHOUT ANY WARRANTY; without even the implied warranty of
	* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
	* GNU General Public License for more details.
	*
	* You should have received a copy of the GNU General Public License
	* along with this program. If not, see <http://www.gnu.org/licenses/>.
	*/

[stale]

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

[stale]

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.