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

Add VIA support for yd60mq #10273

Closed
wants to merge 11 commits into from
Closed

Conversation

DisguisedOtter
Copy link

Description

Add via support for yd60mq. This is without backlight support, as I wasn't comfortable defining 12 or 16 led. It can be enabled easily if you define BACKLIGHT_NUM and change RGBLIGHT_ENABLE to yes. Also add a keymap to use with via

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

none

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

commit 4140a8a471672599da634a3fd2d44122ad4f9601
Author: DisguisedOtter <[email protected]>
Date:   Wed Sep 9 12:06:10 2020 +0200

    add via keymap

commit a49aed8b4588abd803df293605195a4f808c9084
Author: DisguisedOtter <[email protected]>
Date:   Wed Sep 9 12:00:19 2020 +0200

    add via keymap

commit a916bf4d6968dacb9a7074c809e995bc317408c6
Author: DisguisedOtter <[email protected]>
Date:   Wed Sep 9 11:58:24 2020 +0200

    disable rgb

commit 6c4b0a3e00c6cb7206c393ae2696ff2340b99507
Author: DisguisedOtter <[email protected]>
Date:   Wed Sep 9 11:52:19 2020 +0200

    testing layout configuration::wq
@DisguisedOtter DisguisedOtter changed the title Add via support for yd60mq Add VIA support for yd60mq Sep 9, 2020
@DisguisedOtter
Copy link
Author

Anything I need to change? As far as I can tell it builds with the default keymaps, it just fails on some rgb stuff that's not used in the via config

@rbange
Copy link
Contributor

rbange commented Sep 12, 2020

hey, I actually wanted to port this board to via too and found your pr's:

For me (and in CI too) the build fails due to the following error:

Compiling: keyboards/yd60mq/yd60mq.c                                                               In file included from quantum/quantum.h:41,
                 from keyboards/yd60mq/yd60mq.h:3,
                 from keyboards/yd60mq/yd60mq.c:1:
quantum/rgblight.h:232:21: error: 'RGBLED_NUM' undeclared here (not in a function); did you mean 'RGB_HUD'?
 extern LED_TYPE led[RGBLED_NUM];
                     ^~~~~~~~~~
                     RGB_HUD
 [ERRORS]
 |
 |
 |

How did managed to compile your version?

EDIT:
Seems like one need define the RGBLED_NUM and DEVICE_VER within yd60mq/via/config.h. I managed to the get the firmware built and flashed on my 16LED board with:

#define DEVICE_VER 0x0002

#define RGBLED_NUM 16

@DisguisedOtter
Copy link
Author

hey, I actually wanted to port this board to via too and found your pr's:

For me (and in CI too) the build fails due to the following error:

Compiling: keyboards/yd60mq/yd60mq.c                                                               In file included from quantum/quantum.h:41,
                 from keyboards/yd60mq/yd60mq.h:3,
                 from keyboards/yd60mq/yd60mq.c:1:
quantum/rgblight.h:232:21: error: 'RGBLED_NUM' undeclared here (not in a function); did you mean 'RGB_HUD'?
 extern LED_TYPE led[RGBLED_NUM];
                     ^~~~~~~~~~
                     RGB_HUD
 [ERRORS]
 |
 |
 |

How did managed to compile your version?

EDIT:
Seems like one need define the RGBLED_NUM and DEVICE_VER within yd60mq/via/config.h. I managed to the get the firmware built and flashed on my 16LED board with:

#define DEVICE_VER 0x0002

#define RGBLED_NUM 16

Yeah, I mentioned in the PR that those need to be defined for some keymaps, however with the default keymaps it builds without it. Mine doesn't have any backlight so for me there was no way to test and build it with RGB enabled.

@rbange
Copy link
Contributor

rbange commented Sep 14, 2020

Ok. First I made a mistake and checked out your master which probably lead to my build errors.

To clarify: You have an "unmodified" yd60mq without any underglow leds? I am asking, as I've never seen such a model. Following the logic of the old PR #8629. This would result in a new DEVICE_VER aka revision.

Can you provide some information about your board e.g. photos or links?

I have 2 problems with your current implementation:

  1. Defining the DEVICE_VER in the top-level config.h leads to a build error on the 16led revision
  2. Im also uncomfortable with defining an RGBLED_NUM for via so I would recommend following structure:
yd60mq
|-- 12led
|    |-- ...
|-- 16led
|    |-- ...
|-- noled
|    |-- ...
+-- keymaps
    |-- default
    |-- iso
    |-- krusli
    |-- via

This has following advantages:

  1. Defining VIA support in the keymaps/ folder is described in the docs (https://caniusevia.com/docs/configuring_qmk)
  2. Defining VIA within a keymap delegates the RGBLED_NUM problem to its revision.

Maybe we could also give the noled version a different product_id, so we can provide 2 jsons in VIA one w/ lighting tab activated and one w/o?

@DisguisedOtter
Copy link
Author

Ok. First I made a mistake and checked out your master which probably lead to my build errors.

To clarify: You have an "unmodified" yd60mq without any underglow leds? I am asking, as I've never seen such a model. Following the logic of the old PR #8629. This would result in a new DEVICE_VER aka revision.

Can you provide some information about your board e.g. photos or links?

I have 2 problems with your current implementation:

1. Defining the `DEVICE_VER` in the top-level `config.h` leads to a build error on the 16led revision

2. Im also uncomfortable with defining an `RGBLED_NUM` for via so I would recommend following structure:
yd60mq
|-- 12led
|    |-- ...
|-- 16led
|    |-- ...
|-- noled
|    |-- ...
+-- keymaps
    |-- default
    |-- iso
    |-- krusli
    |-- via

This has following advantages:

1. Defining VIA support in the `keymaps/` folder is described in the docs (https://caniusevia.com/docs/configuring_qmk)

2. Defining VIA within a keymap delegates the `RGBLED_NUM` problem to its revision.

Maybe we could also give the noled version a different product_id, so we can provide 2 jsons in VIA one w/ lighting tab activated and one w/o?

Hmm, I made a mistake. I have the 'regular' yd60, which is basically the yd60mq without underglow leds. It uses the same firmware and everything though. Guess it needs to be added as a separate device though. I'll change what's needed and put in a new PR.

@rbange
Copy link
Contributor

rbange commented Sep 14, 2020

Well, nevertheless VIA support for the yd60mq is a desirable feature. Will you add this to your new PR too, or should I open another PR?

Also, if you introduce a new board, I would advice you to put into into the ymdk/ tree. The yd60mq, as well as some mother boards still reside in keyboards/ even though a directory for its manufacturer exists.

@rbange rbange mentioned this pull request Sep 15, 2020
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants