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

Melody96 refactor #16455

Merged
merged 2 commits into from
Mar 4, 2022
Merged

Melody96 refactor #16455

merged 2 commits into from
Mar 4, 2022

Conversation

fauxpark
Copy link
Member

Description

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

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 fauxpark added keyboard keymap via Adds via keymap and/or updates keyboard for via support labels Feb 26, 2022
@fauxpark fauxpark requested review from noroadsleft and a team February 26, 2022 20:29
@drashna drashna requested a review from a team February 26, 2022 21:52
@fauxpark fauxpark merged commit aab2ac2 into qmk:master Mar 4, 2022
@fauxpark fauxpark deleted the melody96-refactor branch March 4, 2022 03:52
@mjparme
Copy link

mjparme commented Jul 13, 2022

@fauxpark Every since this refactor of the melody96 was merged the Enter key is the backslash key and the backslash key is the enter key when using the default keymap. I don't really understand why either because the layouts in melody96.h match the layouts from an older version of melody96.h (QMK Firmware 0.16.8) where using the default keymap doesn't result in the enter and backslash being mixed up.

I can fix it by swapping k93 and k84 in the LAYOUT_all macro (the 1st part of it) but I can't figure out why that would be necessary. Because it used to work fine. Do you have any insight why enter and backslash are mixed up in the default keymap after your refactor?

If you do make ymdk/melody96:default:dfu the enter key and backslash key will be swapped (using LAYOUT_all).

@sigprof
Copy link
Contributor

sigprof commented Jul 13, 2022

@mjparme Do you have a hotswap version of the PCB? AFAIK one weirdness with Melody96 is that the hotswap and soldered versions of the PCB are not 100% compatible — on the hotswap PCB the matrix positions for Enter and backslash are swapped with respect to the same key positions on the soldered PCB. The old code had the LAYOUT_hotswap macro specifically for the ANSI hotswap PCB, which accounted for this difference; however, during the refactoring that layout macro was dropped.

I think that the proper way to fix this would be to split this keyboard into two separate versions — maybe ymdk/melody96/soldered and ymdk/melody96/hotswap, and make sure that each of those versions has accurate layout macros. But doing that requires having the actual PCBs for testing. The fact that the Melody96 hotswap PCB also has multiple layout options (configurable by choosing which locations for the hotswap sockets to populate) also complicates things. Also apparently the hotswap PCB now has two different revisions (“Hotswap 96 V1” and “Hotswap 96 V2”); not sure whether they have any important differences except switch orientations.

@mjparme
Copy link

mjparme commented Jul 13, 2022

@sigprof I do not have the hotswap version of the PCB. I have two of these: https://www.aliexpress.com/item/2251832660403665.html. One I bought in Feb 2021 and the other I bought in Nov 2021. Both are experiencing the swapped backslash and enter key when using the default keymap (which uses the LAYOUT_all macro) from the master branch.

If I instead use an older branch that I have that is at commit 675ce76 (it reports itself as QMK Firmware 0.16.8 during compilation) the backslash and enter keys are not swapped when using the default keymap. Doing a diff between LAYOUT_all in master branch and my older branch shows no differences in the macro.

It is very puzzling to say the least.

I have created a LAYOUT_backslash_enter_swapped macro where I have swapped k93 and k84 and this works but for the life of me I can't figure out why this was necessary.

@noroadsleft
Copy link
Member

noroadsleft commented Nov 2, 2022

@mjparme I've had your report kicking around in the back of my mind, intending to investigate your report. I finally went back to investigate this by checking out the commit previous to this PR, and doing my own refactor from that.

I'm fairly confident that this PR switched the Backslash and Enter locations when it should not have. My reasoning for this, in addition to the refactor I did locally, is the fact that the commit that brought this PR's changes into QMK's mainline has position k93 above position k84, but the previous commit has the corresponding position K084 above K093.

I also was able to look up a Discord exchange with a user who had this board (I don't know if they still do). Their keymap is in the repo (the konstantin keymap), and in the Discord exchange they confirmed that their board was built using an ANSI Enter. I cross-referenced their keymap with the layout macro QMK had at the time, and confirmed that macro had position K084 above K093 as well.

My thoughts are:

I also just checked the VIA support. It seems the keys in question have had their positions switched three times since support was initially added. I suspect the users in question have different revisions of the board, and that none of them talked to each other. I also note that the VIA data only supports seems to support the Hotswap PCB. (edit: not accurate; I checked the wrong data)

It'd be a help if @NoFunDon and @vinorodrigues could specify which versions of the Melody96 they own(ed).

@NoFunDon
Copy link

NoFunDon commented Nov 2, 2022

I have the hotswap version of the Melody96

@vinorodrigues
Copy link
Contributor

vinorodrigues commented Nov 3, 2022

Original solder version for me. Having said that - I'd used the Melody96 codebase to create the idobao/id96 code, as I know that ID OEM's from YMDK on this PCB. For the solder version (that I still have and use) I can confirm that K84 is the [ \ ] key and K93 is the [Enter].

@noroadsleft - It could be that Hotswap and Solder are indeed different and thus the Melody should have 2 different code "revisions".

@noroadsleft
Copy link
Member

Thanks for the replies.

At the very least, I think on the VIA side this would require two different JSON data files, because making the data valid for one (either Hotswap or Soldered) seems to make the data invalid for the other. The QMK codebase would also need multiple revisions to differentiate the Vendor/Product ID pairings, though technically it should have that anyway – the QMK guideline is "different PCBs should be added as different keyboards," though that wasn't well-enforced in the early days.

@NoFunDon, do you happen to have a product link for the PCB/keyboard you bought? I searched melody 96 on AliExpress (knowing YMDK has an active storefront there) and found two different hotswap PCBs: A "Melody 96 Pro" with per-key RGB, and a "Hotswap Melody 96 Kit 1" with a different PCB that's more like a soldered PCB with hotswap sockets pre-installed (buyer is offered the choices of ANSI or ISO, and 3x 1u or 2x 1.5u mods on the right side).

Additionally, there are at least two versions of the Soldered PCB: one with a USB Micro B connector, and one with a Type C. Fortunately, I have no reason to believe there's any difference in them besides the connector.

@mjparme, does your Melody96 PCB have a Type C connector?

@vinorodrigues
Copy link
Contributor

vinorodrigues commented Nov 3, 2022

Well ... if you skip Ali and go direct to ymdkey dot com you get 4 "versions":

  • YMD-96 Soldering PCB
  • YMD-96 Hotswap QMK Underglow PCB (V1 South Facing)
  • YMD-96 Hotswap QMK Underglow PCB (V2 North Facing)
  • YMD-96 V3 RGB Hotswap PCB (a.k.a. Melody 96 Pro)

( FYI: @noroadsleft ) ... and that's the current avails.

@mjparme
Copy link

mjparme commented Nov 3, 2022

@noroadsleft my PCBs have a USB-C connector

@noroadsleft
Copy link
Member

Okay, here's some firmware for testing. Both of these should have proper Enter key placement (I hope!).

@mjparme and @vinorodrigues,

If you're willing, please flash the appropriate firmware file to your Melody96s and sideload the VIA JSON files (which are V2 spec; I don't know V3 spec yet), and let me know if they work as they should. Both firmware files are VIA enabled, and the top right key on the default layer is QK_BOOT (aka RESET; jump to bootloader).

Firmware VIA JSON
ymdk_melody96_hotswap_nrl_dev.hex.txt melody96_hotswap.json.txt
ymdk_melody96_soldered_nrl_dev.hex.txt melody96_soldered.json.txt

Also @mjparme,

I asked you the wrong question previously, forgetting at the time that your board is Hotswap. A more appropriate question is: does your PCB have South-facing LEDs footprints, or North-facing? I'm wondering how confident to be about the V1 and V2 Hotswap boards having the same matrices.

@vinorodrigues
Copy link
Contributor

vinorodrigues commented Nov 6, 2022

ok tested it .. enter and | are correctly oriented

@mjparme
Copy link

mjparme commented Nov 6, 2022

Also @mjparme,

I asked you the wrong question previously, forgetting at the time that your board is Hotswap. A more appropriate question is: does your PCB have South-facing LEDs footprints, or North-facing? I'm wondering how confident to be about the V1 and V2 Hotswap boards having the same matrices.

@noroadsleft My PCB is not hotswap. It is soldered.

I have also never used VIA before. I have gone to it’s web page before but it had no information at all about what it was or what it did. Just said “your keyboards best friend”. But didn’t say why or how it was my keyboards best friend. So I have no idea what it does.

@noroadsleft
Copy link
Member

@mjparme, My apologies. I got your board mixed up with NoFunDon's. Disregard the testing firmware and VIA instructions – vinorodrigues was able to verify the data I had for the Soldered PCBs.

@NoFunDon, does your PCB have South-facing LEDs footprints, or North-facing? If you're willing, please download the testing firmware and VIA JSON I posted here and let me know:

  1. if your Enter and Backslash keys behave as they should
  2. if the VIA data is accurate for your board.

@noroadsleft noroadsleft mentioned this pull request Sep 22, 2023
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keyboard keymap via Adds via keymap and/or updates keyboard for via support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants