Fixed: inconsistent MATRIX_COLS and MATRIX_COL_PINS in facew.

[elfmimi]

Description

The fix compiles OK. But I don't have the corresponding hardware so I cannot actually test it.
TBH, I have a feeling that this code has no user at all.

I need consistency of MATRIX_ROWS and MATRIX_COLS against MATRIX_ROW_PINS and MATRIX_COL_PINS respectively for all keyboards to shape my next PR complete.

Could you help? @mechmerlin

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

• [Bug] Inconsistent MATRIX_COLS and MATRIX_COL_PINS. (KB: facew) #8106

Checklist

• My code follows the code style of this project.
• 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).

[mechmerlin]

Hey thanks for the submission, however I do have some issues with this

1. Typically its not a good idea to change pins on firmware when you don't have hardware to actually test this on. I no longer have the board, but with the current firmware, it was my work board for a good 2 years. Unfortunately I cannot test your change either.

2. All but 1 bootmapperclient type boards, use A0, B0 as the reset switch. This is typically programmed in QMK as the Row 0 and Col 0, so that K00 is the location of the reset switch. This disrupts it.

3. One reason why winkeyless.kr closed, is people just rampantly copied their boards without permission. FaceW is a copy of the b.face with a few things different. Both boards have since been copied many times over. I have encountered boards that are named facew, but when flashed with this firmware they fail. My FaceW is direct from Sprit himself.

I am not in favor of this change.

[elfmimi]

Thank you for reviewing! I very much appreciate those information you provided.

I was hoping that you have this keyboard in possession and you could test it.
And the situation was more complicated than I thought.
While the code for this keyboard appears as a simplest of kind, that BootMapperClient thing adds complexity which we don't want to handle now.

I'll rethink about what to do.

[zvecr]

Given the missing column, it would imply the board doesn't work right now anyway.

Removing the empty column wont break anything, given it physically had no way to trigger something like bootmagic lite (any other behaviour would have to be within the bootloader which we don't control).

[elfmimi]

I did survey and I couldn't find out any proof that the binary of qmk-firmware built from this code is meant to be used in conjunction with Bootmapper Client.

I'm guessing this only serves as a placeholder before it gets overwritten with the binary equipped with Bootmapper Client.

[elfmimi]

Minimum change for minimum impact.
Please merge this.

If there is a real need for removing vacant columns in the matrix, facew is not the only one that should get treated. So it should be handled in a separate issue/pull-request.
One example is bfake keyboard. https://github.com/qmk/qmk_firmware/blob/master/keyboards/bfake/bfake.h

[elfmimi]

... closing and reopening was unintentional.

It's been stale, so I'd like to state again that current code is apparently wrong and it's not functioning and it's inconsistency is blocking another PR of mine. I need this merged.

[elfmimi]

branch re-synchronized to the new master.

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

[drashna]

LGTM. But I woulg prefer that mechmerlin signs off on this, before getting merged.

[tzarc]

Ultimately, something needs to be fixed, as we have #define MATRIX_COLS 11 but MATRIX_COL_PINS only has 10 entries on master. Can we find someone who does have this board?

[giammin]

I have this board! facew sprit edition ps2avru v1.0

[elfmimi]

@giammin Thank you for making contact with us. Are you using QMK as a firmware for your facew?

[giammin]

not yet, I was looking if I could swap bootmapper client with qmk

if it was stable for daily use

if I don't risk to fry my board I can do some tests

[giammin]

@elfmimi i had some time to test it: i tried flashing the default layout and it does not work

if you tell me what to change i can test if you are right about the columns count

[elfmimi]

@giammin Thank you for so much kindness helping us out. If it does not work I believe unmodified build should also be not working. If that is the case we need to find out why.
This is the point where this keyboard last got major change. #8106 (comment)

1. It could have already been broken at this point.
2. Something must have broke it since then to date.

[giammin]

i found this PR #2493

[giammin]

@elfmimi good news! As expected with this modify it works

[elfmimi]

@giammin Fantastic. Ok then let us reorganize information.
You said it didn't work when you tried for the first time. That one was the unmodified one. is this correct?
Next. The modification you tried and successfully worked is identical to that of this pull-request's latest commit. Please confirm this.

[giammin]

You said it didn't work when you tried for the first time. That one was the unmodified one. is this correct?

yes

The modification you tried and successfully worked is identical to that of this pull-request's latest commit. Please confirm this.

yes! just added C5 on the pin list:

#define MATRIX_COL_PINS { A0, A1, A2, A3, A4, A5, A6, A7, C7, C6 , C5 }

[elfmimi]

Let's merge this. @ drashna fauxpark zvecr tzarc