New controller board 'kintwin' for Kinesis Contoured keyboards

[alvicstep]

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.
• [x ] 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).

[alvicstep]

I have tested this change. Build options in info.json are now overridden by rules.mk in kinesis folder. I got warnings when building.

I see that some developers define build options at keymap level (keymaps/default/rules.mk), others at controller level (nguyenvietyen/rules.mk)

It seems that priority rules for rules.mk files is that the build options in a child folder take priority over the build options in parent folder. It works differently when build options are moved to info.json.

Options:

1. Each controller in kinesis folder should be able to define its own build options. But we already have build options defined in rules.mk under kinesis folder. It would be too risky to remove this file now because some controllers might rely on these build options. I suggest to continue using rules.mk files at different levels to define build options rather than trying to get it to work with info.json

[dunk2k]

Would the build warnings be indicative that each controller/board under kinesis dir should have their own rules.mk/info.json defining what feature are enabled/disabled, which is best practise, rather than having a blanket/master rules.mk?

[dunk2k]

Linting check is failing due to no licence headers in identified keymaps.

[alvicstep]

Would the build warnings be indicative that each controller/board under kinesis dir should have their own rules.mk/info.json defining what feature are enabled/disabled, rather than having a blanket/master rules.mk?

Yes, I think master rules.mk is unnecessary. But its removal might result in build option changes for some controllers.

[alvicstep]

What files are missing license headers?

[dunk2k]

What files are missing license headers?

kinesis/keymaps/default/keymap.c and kinesis/keymaps/default_pretty/keymap.c

[dunk2k]

Yes, I think master rules.mk is unnecessary. But its removal might result in build option changes for some controllers.

Having had a look at kinesis/rules mk, thankfully it doesn't contain definitions nor dependencies that would break controllers, so if the master rules.mk is removed then rules.mk or info.json for each controller would need to be updated - conforming to best practise.

[alvicstep]

Committed requested changes

[dunk2k]

Committed requested changes

excellent! I'm not sure why linting checks are still failing 😳

edit: figured it out. For good measure, I'd advise the fix is applied to all keymap.c files scoped in this PR 🙂

[alvicstep]

I have tested this change. Build options in info.json are now overridden by rules.mk in kinesis folder. I got warnings when building.

I see that some developers define build options at keymap level (keymaps/default/rules.mk), others at controller level (nguyenvietyen/rules.mk)

It seems that priority rules for rules.mk files is that the build options in a child folder take priority over the build options in parent folder. It works differently when build options are moved to info.json.

Options:

1. Each controller in kinesis folder should be able to define its own build options. But we already have build options defined in rules.mk under kinesis folder. It would be too risky to remove this file now because some controllers might rely on these build options. I suggest to continue using rules.mk files at different levels to define build options rather than trying to get it to work with info.json

[alvicstep]

I see build errors. It seems these issues were already in the code base even before my PR.

I see a lot of issues from ericgebhart dir, like this one:

users/ericgebhart/extensions/keycodes.h:35:47: error: unknown type name 'keyrecord_t'; did you mean 'keyevent_t'?
bool process_record_secrets(uint16_t keycode, keyrecord_t *record);

How do you usually go about fixing them? Do you try reach out to the maintainer or this is a responsibility of the last person commiting the code?

[dunk2k]

Those are either keymaps or userspace keymaps that have bad code in them. For adding a new keyboard, the important keymap that is require to pass CI build checks is default (and any "default" community layouts added).

You're not required to fix keymaps or userspace keymaps that aren't yours 🙂
You're correct in that the individual who created the keymap or userspace keynap is principally responsible for fixing their code.

The CI build check helps reviewers determine bad PRs, so a PR with a failed CI build isn't automatically disqualified/ignored.

[alvicstep]

Great, thanks

[laffed]

@dunk2k @alvicstep
any update on this MR?
workflow is failing on unrelated/existing problems in other keymaps correct?

[dunk2k]

@laffed
My previous comment, starting "Those are either", still applies.

[laffed]

@dunk2k so to recap,

1. due to some kinesis directory changes, some of the keymaps are now failing build, but not any defaults (that i see)
2. the failing keymaps are not part of the scope of this MR (other maintainers)

So in order for this MR to go through, either @alvicstep would need to fix the failures in this MR or wait until other maintainers fix problems with their own builds.

But these builds are only failing now in this MR, so these maintainers would not even know this is an issue

[dunk2k]

@laffed

The CI build check helps reviewers determine bad PRs, so a PR with a failed CI build isn't automatically disqualified/ignored.

[laffed]

@dunk2k

@laffed

The CI build check helps reviewers determine bad PRs, so a PR with a failed CI build isn't automatically disqualified/ignored.

forgive me, im pretty ignorant here. just a keyboard user who is using the kintwin

to be crystal clear are you saying:
A. This MR is failing builds, but can be merged in anyways?
B. This MR still needs work by @alvicstep directly related to the features he/she introduced?
C. This MR is waiting for upstream changes by other maintainers before it can be merged?

Who's court is this MR ball currently in?

[dunk2k]

forgive me, im pretty ignorant here. just a keyboard user who is using the kintwin

to be crystal clear are you saying: A. This MR is failing builds, but can be merged in anyways? B. This MR still needs work by @alvicstep directly related to the features he/she introduced? C. This MR is waiting for upstream changes by other maintainers before it can be merged?

Who's court is this MR ball currently in?

A. Yes a Pull Request that fails CI Build can still be merged.
B. There aren't any pending change requests so no further changes/commits are required.
C. This PR has been approved by 1 collaborater and has been passed onto the collaborator team for their review.

Current status of this PR is it's ready for review and to be merged by collaborator(s), i.e. ball's in collaborator's court.

[laffed]

@fauxpark bumping kindly

[drashna]

This is a nitpick, so feel free to ignore, but this bothers me.

Specifically, most/all keycodes have a 7 character name/alias, so you can have a 9 wide column (eg 1234567, ), so you can have consistent formatting/placement. So this scrunched together could be better spaced.

[fauxpark]

Please remove this layout and replace it with LAYOUT_pretty.

[alvicstep]

"Layout" matrix was added to have the widest compatibility with the existing keymaps. For example, if I remove "layout" matrix then the controller will stop working with "default" keymap.

Also, other controllers like kint41 or stapelberg and others implement both matrices.

Can you please explain your reasoning for removing "layout" matrix or did I misunderstand your intention?

[fauxpark]

See this PR: #21384

[alvicstep]

I would like to understand the request better.

You would like to:

1. remove LAYOUT

2. create LAYOUT alias to point to LAYOUT_pretty

   Something like that:
      "layout_aliases": {
          "LAYOUT": "LAYOUT_pretty"
      },
   

Is it correct?

I looked at the PR you referenced. "Split" layout (or layout_pretty in our case) is easier to read and as you put it, it has "more conventionally correct ordering". And I think it is very reasonable rule to follow for new keyboards which have no existing keymaps from other contributors. And if you insist I will definitely remove "stack" layout from kintwin controller configuration. However, as I already mentioned, this will make this controller incompatible with a lot of keymaps.

In fact, I went through every keymap under "keymaps" directory and found that there are

• 10 keymaps which use "stack" layout: default, dvorak, dvorak_nguyenvietyen, farmergreg, heatxsink, jwon, peott-fr, stapelberg, tw1t611, xyverz

• only 3 keymaps which use "split" layout: default_pretty, insertsnideremarks, tuesdayjohn

Other contributors did a lot of work by creating these keymaps and I would like to take advantage of this work, if possible. Also, this puts kintwin controller at a disadvantage because every other controller does define both "layout" and "layout_pretty" layouts.

[fauxpark]

No, LAYOUT should be removed, and LAYOUT_pretty renamed to just LAYOUT.
And yes, that means this PR would be blocked until those keymaps can be updated.

[laffed]

@fauxpark it's unclear without digging whether your pr makes updates to keymaps that you yourself created.
what is your response to the question above?
by your last comment do you mean to say something like "i made updates to other peoples work, so you can too"?

your PR also only makes changes to existing keymaps (again unclear whether exclusively yours, exclusively not yours, or mix). which seems more like a code improvement PR /refactor.

the scope of this PR is a new feature addition.

again the question is: should it be that new PR's for new features have to fix previously introduced errors before they can be merged? that seems both way out of scope for one PR, and a pretty big issue for open PR congestion

[fauxpark]

Yes, it is out of scope. I will sort out the user keymaps.

[laffed]

@fauxpark looks like @alvicstep made changes making this conversation obsolete .
can we resolve this conversation and re-review? or are you still in the process of making commits to this branch or separate branch with the above mentioned changes?

[fauxpark]

The keymaps have been updated. #21569

[alvicstep]

Based on the changes in 21569 by @fauxpark I have moved kintwin directory back to kinesis directory.

Because 21569 PR was created off of "develop" branch I created new branch from "develop" branch as well and created new PR: 21614.

@fauxpark , please review

P.S.
I am closing this PR. Hopefully new PR will be more lucky

[alvicstep]

It is very unlikely that 10 different contributors are going to convert their keymaps to a "split" layout. They are not even aware of this effort.

Even if I can fix these 10 keymaps, I can only test them on kintwin controller. There is no guarantee they will work with the original controllers. Also, some of these keymaps are very elaborate and implement multiple layers. Converting them to a "split" design will be a very time consuming operation.

Options:

1. I can move out kintwin controller from Kinesis directory, so it will be treated as a separate keyboard. This is the approach taken by the contributor who designed "kin80" controller. This will reduce the discoverability of Kinesis based controllers and eliminate the possibility of sharing key maps among all the controllers. But it seems to be a reasonable path to move forward under these new guidelines
2. ?

[alvicstep]

It is very unlikely that 10 different contributors are going to convert their keymaps to a "split" layout. They are not even aware of this effort.

Even if I can fix these 10 keymaps, I can only test them on kintwin controller. There is no guarantee they will work with the original controllers. Also, some of these keymaps are very elaborate and implement multiple layers. Converting them to a "split" design will be a very time consuming operation.

Options:

1. I can move out kintwin controller from Kinesis directory, so it will be treated as a separate keyboard. This is the approach taken by the contributor who designed "kin80" controller. This will reduce the discoverability of Kinesis based controllers and eliminate the possibility of sharing key maps among all the controllers. But it seems to be a reasonable path to move forward under these new guidelines
2. ?

I have implemented the option I have described above. I hope this is an acceptable solution. I will be happy to merge kintwin controller files back into "kinesis" directory as soon as the issue with keymaps is resolved.