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

Lotus58 glow #20386

Merged
merged 21 commits into from
Apr 15, 2023
Merged

Lotus58 glow #20386

merged 21 commits into from
Apr 15, 2023

Conversation

TweetyDaBird
Copy link
Contributor

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

keyboards/tweetydabird/lotus58/config.h Outdated Show resolved Hide resolved
keyboards/tweetydabird/lotus58/info.json Show resolved Hide resolved
keyboards/tweetydabird/lotus58/info.json Outdated Show resolved Hide resolved
keyboards/tweetydabird/lotus58/lotus58.c Outdated Show resolved Hide resolved
keyboards/tweetydabird/lotus58/lotus58.c Outdated Show resolved Hide resolved
TweetyDaBird and others added 4 commits April 10, 2023 20:38
Co-authored-by: jack <0x6a73@protonmail.com>
Co-authored-by: jack <0x6a73@protonmail.com>
Co-authored-by: jack <0x6a73@protonmail.com>
Copy link
Member

@zvecr zvecr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keymap folder per revision seems a bit unnecessary given its the same PCB in all cases.

keyboards/tweetydabird/lotus58/keymaps/default/keymap.c
keyboards/tweetydabird/lotus58/atmeldfu/rules.mk
keyboards/tweetydabird/lotus58/caterina/rules.mk
keyboards/tweetydabird/lotus58/nanoboot/rules.mk

Using some existing terminology could also help with consistency;

  • caterina -> promicro
  • atmeldfu -> elite_c

As a decent proportion of user will have no idea when to use caterina vs atmeldfu.

keyboards/tweetydabird/lotus58/info.json Outdated Show resolved Hide resolved
keyboards/tweetydabird/lotus58/info.json Outdated Show resolved Hide resolved
keyboards/tweetydabird/lotus58/info.json Outdated Show resolved Hide resolved
@zvecr zvecr mentioned this pull request Apr 11, 2023
14 tasks
@TweetyDaBird
Copy link
Contributor Author

TweetyDaBird commented Apr 11, 2023

Keymap folder per revision seems a bit unnecessary given its the same PCB in all cases.

keyboards/tweetydabird/lotus58/keymaps/default/keymap.c
keyboards/tweetydabird/lotus58/atmeldfu/rules.mk
keyboards/tweetydabird/lotus58/caterina/rules.mk
keyboards/tweetydabird/lotus58/nanoboot/rules.mk

Using some existing terminology could also help with consistency;

  • caterina -> promicro
  • atmeldfu -> elite_c

As a decent proportion of user will have no idea when to use caterina vs atmeldfu.

Well, since it is in fact categorized by boot loader, and the list of controllers it applies to was included in the readme. But sure I can rename them.

Edit: to clarify, since Atmega32u4 are routinely running out of memory for advanced features on split keyboards especially, I try to steer builders to use nanoBoot as the bootloader, and for whatever reason, making commented out lines in a single firmware ended up causing confusion. Separate folders/versions seems to solve/reduce that.

TweetyDaBird and others added 5 commits April 11, 2023 08:16
Co-authored-by: jack <0x6a73@protonmail.com>
Co-authored-by: Joel Challis <git@zvecr.com>
Co-authored-by: Joel Challis <git@zvecr.com>
Co-authored-by: Joel Challis <git@zvecr.com>
@TweetyDaBird
Copy link
Contributor Author

TweetyDaBird commented Apr 13, 2023

I believe all requested changes (manual edits) should be fixed and tested now. Let me know if there is more that needs fixing.

Again, in all respect, no rush, just confirming that I have tested things.

@TweetyDaBird TweetyDaBird requested a review from zvecr April 13, 2023 05:23
@zvecr
Copy link
Member

zvecr commented Apr 13, 2023

Keymap folder per revision seems a bit unnecessary given its the same PCB in all cases.

keyboards/tweetydabird/lotus58/keymaps/default/keymap.c
keyboards/tweetydabird/lotus58/atmeldfu/rules.mk
keyboards/tweetydabird/lotus58/caterina/rules.mk
keyboards/tweetydabird/lotus58/nanoboot/rules.mk

This suggestion has not been completed.

@TweetyDaBird
Copy link
Contributor Author

TweetyDaBird commented Apr 13, 2023

Ah, right. Then I misunderstood and/or didn't read carefully enough. Now, I'll have to think on that. This sort of creates an issue going to Vial.

Again trying to have a discussion, not arguing/being a PITA more than necessary. I agree that this isn't your issue to solve, I'm just trying to find the middle ground where it's acceptable here, and doesn't totally shoot me in the foot there, creating more work as a whole. Feel free to just ignore me if you don't want to have the discussion.

Moving the key-maps to a central folder doesn't work in Vial as the key-maps there have to be dependent on the boot-loader (lack of space) and the standard for the Vial key-map is a single folder with the name Vial.

Do you have any good suggestions for a workaround?

Can the boot-loader assignment move to key-map level? It actually can can't it? (Although I'm not sure if that the correct thing to do).

@zvecr
Copy link
Member

zvecr commented Apr 13, 2023

where it's acceptable here, and doesn't totally shoot me in the foot there

The only thing that is of concern here if its acceptable to QMK. Optimising, even if its only in part, for Vial is zero on the scale.

However it should be noted that having a common folder for keymaps does not stop at a later date the introduction of

keyboards/tweetydabird/lotus58/atmeldfu/keymaps/something/keymap.c
keyboards/tweetydabird/lotus58/caterina/keymaps/something/keymap.c

As long as "something" doesnt also exist within keyboards/tweetydabird/lotus58/keymaps.

You can also use have conditional blocks within the compilation units, with #ifdef BOOTLOADER_CATERINA and things like post_rules.mk to disable features completely.

@TweetyDaBird
Copy link
Contributor Author

The only thing that is of concern here if its acceptable to QMK. Optimising, even if its only in part, for Vial is zero on the scale.

I fully understand the position. However, had I just swallowed the first suggestion and not questioned, it would probably resulted in another PR with an update fairly soon, and then another, and another... Making us both batty...

The reason I asked however is this:

You can also use have conditional blocks within the compilation units, with #ifdef BOOTLOADER_CATERINA and things like post_rules.mk to disable features completely.

That should have been obvious to me, having used the '#ifdef OLED' etc... It wasn't for some reason. So, from the bottom of my heart, thank you for making me feel damned stupid... That was going beyond...

It might perhaps take me a while to iron it out to my satisfaction (and yours), but I think you just gave me the key...

@drashna
Copy link
Member

drashna commented Apr 14, 2023

This is the fifth PR for this board: #20281, #18554, #15867, #12594.

@TweetyDaBird
Copy link
Contributor Author

This is the fifth PR for this board: #20281, #18554, #15867, #12594.

Yeah. I don't do good with GitHub time limits and such. ADHD. Either things happen fast, and I say focused, or I loose focus and hyper focus on something else, and end up procrastinating. Was sort of why I managed to annoy Joel before(justified from his side Btw). I was more trying to keep myself on task than anything else.

And git isn't my strong suit to start with. Is there a way to re-open a PR from my side? I would have preferred that.

@TweetyDaBird
Copy link
Contributor Author

TweetyDaBird commented Apr 14, 2023

Well, a work days worth of testing. It work as is, and armed with the above tip, I feel I can take this and make Vial work without shooting myself in the foot once it's in the main repo. I'm happy with it. Let me know if there is anything else that needs fixed.

keyboards/tweetydabird/lotus58/elite_c/rules.mk Outdated Show resolved Hide resolved
keyboards/tweetydabird/lotus58/info.json Outdated Show resolved Hide resolved
keyboards/tweetydabird/lotus58/keymaps/default/keymap.c Outdated Show resolved Hide resolved
keyboards/tweetydabird/lotus58/promicro/rules.mk Outdated Show resolved Hide resolved
TweetyDaBird and others added 4 commits April 15, 2023 09:54
Co-authored-by: Pablo Martínez <58857054+elpekenin@users.noreply.github.com>
Co-authored-by: Pablo Martínez <58857054+elpekenin@users.noreply.github.com>
Copy link
Member

@zvecr zvecr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ive fixed up things after the bad info you were given, as well as a few minor tidy ups. All good to go from my front now.

@keyboard-magpie keyboard-magpie merged commit 28c11ed into qmk:master Apr 15, 2023
sudish added a commit to sudish/qmk_firmware that referenced this pull request Apr 16, 2023
* upstream/master: (73 commits)
  [Keyboard] Add Kalakos Bahrnob65 (qmk#20424)
  Tidy up stray RGB_DISABLE_TIMEOUT references (qmk#20460)
  [Keyboard] Add zoom75 wired (qmk#20396)
  [Keyboard] Add dymium65 (qmk#20257)
  Lotus58 glow (qmk#20386)
  ADPenrose Obi Layout Macro Conversion and Addition (qmk#20445)
  Add hardware information momokai keyboards (qmk#20434)
  4pplet/eagle_viper_rep/rev_a Layout Macro Conversion and Additions (qmk#20414)
  [Keymap] Add paulomp90 lily58 keymap (qmk#20327)
  [Keymap] Add personal keymap for Lily58 (qmk#18735)
  [Keyboard] Fix h87 g2 VID conflict (qmk#20388)
  [Keymap] PHSC138 Keymap for Atom47 (qmk#18768)
  [Keyboard] add kb2040 flavor of gherkin (qmk#18360)
  [Keyboard] ymdk/id75 (qmk#19967)
  fixing bug that caused KC_DEL and KC_MUTE (encoder press) to be swapped (qmk#20420)
  4pplet/bootleg/rev_a Layout Macro Conversion and Addition (qmk#20400)
  4pplet/aekiso60 Layout Macro Conversion and Additions (qmk#20399)
  Reject info.json at keymap level (qmk#20408)
  Bump anothrNick/github-tag-action from 1.61.0 to 1.62.0 (qmk#20407)
  [Keyboard] Update angle65 VID/PID (qmk#20401)
  ...
@TweetyDaBird
Copy link
Contributor Author

Ive fixed up things after the bad info you were given, as well as a few minor tidy ups. All good to go from my front now.

Thanks! And a lot of thanks for the pointers!

rodrigob pushed a commit to rodrigob/qmk_firmware that referenced this pull request Apr 18, 2023

Co-authored-by: jack <0x6a73@protonmail.com>
Co-authored-by: Joel Challis <git@zvecr.com>
Co-authored-by: Pablo Martínez <58857054+elpekenin@users.noreply.github.com>
quangd42 pushed a commit to quangd42/qmk_firmware that referenced this pull request Apr 20, 2023

Co-authored-by: jack <0x6a73@protonmail.com>
Co-authored-by: Joel Challis <git@zvecr.com>
Co-authored-by: Pablo Martínez <58857054+elpekenin@users.noreply.github.com>
queyenth pushed a commit to queyenth/qmk_firmware that referenced this pull request Apr 21, 2023

Co-authored-by: jack <0x6a73@protonmail.com>
Co-authored-by: Joel Challis <git@zvecr.com>
Co-authored-by: Pablo Martínez <58857054+elpekenin@users.noreply.github.com>
stratosgear pushed a commit to stratosgear/qmk_firmware that referenced this pull request Apr 26, 2023

Co-authored-by: jack <0x6a73@protonmail.com>
Co-authored-by: Joel Challis <git@zvecr.com>
Co-authored-by: Pablo Martínez <58857054+elpekenin@users.noreply.github.com>
@TweetyDaBird TweetyDaBird deleted the Lotus58Glow branch April 29, 2023 16:21
struckmb pushed a commit to struckmb/qmk_firmware that referenced this pull request May 1, 2023

Co-authored-by: jack <0x6a73@protonmail.com>
Co-authored-by: Joel Challis <git@zvecr.com>
Co-authored-by: Pablo Martínez <58857054+elpekenin@users.noreply.github.com>
SjB added a commit to SjB/qmk_firmware that referenced this pull request May 12, 2023
* master:
  NK Plus (qmk#20392)
  [Docs] Fix suggested code pattern when a specific mod-mask is required. (qmk#20512)
  [Docs] Remove combo count from array (qmk#20511)
  Add QuadrumLabs Delta (qmk#20409)
  Adds Docs option for ArduinoIDE's example `ArduinoISP` (qmk#20486)
  GMMK 2 volume up/down Fn keys are backwards in default mapping (qmk#20476)
  Fix typo in `feature_wpm.md` title (qmk#20464)
  [Keyboard] Add Kalakos Bahrnob65 (qmk#20424)
  Tidy up stray RGB_DISABLE_TIMEOUT references (qmk#20460)
  [Keyboard] Add zoom75 wired (qmk#20396)
  [Keyboard] Add dymium65 (qmk#20257)
  Lotus58 glow (qmk#20386)
  ADPenrose Obi Layout Macro Conversion and Addition (qmk#20445)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants