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

Adding alternative configs for Piantor and update example #21452

Closed
wants to merge 4 commits into from

Conversation

duncanam
Copy link

@duncanam duncanam commented Jul 4, 2023

Description

Here we add an entrypoint for the Weact controller which is an option that Beekeeb ships with their Piantor kits, as well as the required variable to define which vbus pin the vanilla Pico uses vs. the Weact Pico. Additionally, added an example flashing with the Weact config as well as an alternative layout, Miryoku (this was originally puzzling for me, and this would have been helpful).

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

  • enable Weact for Piantor
  • add example
  • adding "no vbus sense" for microcontrollers without that mode assembled in the Piantor

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). (I'm currently typing this using this change)

@duncanam
Copy link
Author

duncanam commented Jul 4, 2023

@l4u Mind reviewing?

@dunk2k
Copy link
Contributor

dunk2k commented Jul 5, 2023

In readme.md could you add a paragraph with conditions for when to compile no_vbus_sense?

@duncanam
Copy link
Author

duncanam commented Jul 5, 2023 via email

@duncanam
Copy link
Author

duncanam commented Jul 6, 2023

@dunk2k and/or @drashna - what is this repo's policy on squashing commits during PR? Would you like me to add commits with edits then squash at the end, squash as I add, or don't squash at all and leave a full paper trail (albeit messier) upon merge with master?

@drashna
Copy link
Member

drashna commented Jul 6, 2023

@dunk2k and/or @drashna - what is this repo's policy on squashing commits during PR? Would you like me to add commits with edits then squash at the end, squash as I add, or don't squash at all and leave a full paper trail (albeit messier) upon merge with master?

We squash when we merge PRs. So the commit history from the PR doesn't really matter.

Though, personally, I prefer that it's not "cleaned up", since it makes it easier to see when and where changes occurred. But that's just my preference.

@duncanam
Copy link
Author

duncanam commented Jul 6, 2023

TODO for me:

  • add no vbus sense example
  • update with copyright header

@duncanam duncanam requested a review from drashna July 6, 2023 06:52
@duncanam
Copy link
Author

duncanam commented Jul 6, 2023

Build failing on bcat targets here- I may need some guidance on how best to proceed to fix.

@dunk2k
Copy link
Contributor

dunk2k commented Jul 6, 2023

Build failing on bcat targets here- I may need some guidance on how best to proceed to fix.

Thankfully I've already a PR (#21205) for fixing that.
Regardless, as it's a userspace+community layout, it wouldn't be your remit/responsibility to fix 😉

@duncanam duncanam requested a review from drashna July 15, 2023 15:51
@duncanam
Copy link
Author

@l4u just wanted to also check in with you on this one too

@duncanam
Copy link
Author

@drashna Any other suggestions here?

@drashna drashna requested a review from a team July 28, 2023 06:43
Copy link
Member

@fauxpark fauxpark left a comment

Choose a reason for hiding this comment

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

This should really be inverted, that way there is no need to undef. And keyboard updates need to go through develop.

@duncanam
Copy link
Author

@fauxpark How do we invert such that the default config has that pin defined but the two special configs have variations (either a new pin in the case of the Weact microcontroller, or no pin, for that case)?

@fauxpark
Copy link
Member

Probably the easiest would be to split the board into two "revisions" instead of how you've done it here. Then make sure the "base" cannot be built, either by removing the rules.mk or setting DEFAULT_FOLDER to the revision corresponding to the current config.

@duncanam
Copy link
Author

duncanam commented Jul 28, 2023

@fauxpark Okay let's see if I am understanding this: we can remove the pin definition from the current "base" config and place this into a new folder, "base", with its own rules, and define a new constant DEFAULT_FOLDER in the root definition that points to "base", such that when building the config it will default to "base" if not specified?

@fauxpark
Copy link
Member

Basically. You'd have two subfolders named say pico and weact, both with empty rules.mk (to make them build targets), but one also has a config.h with the VBUS pin define. Then you add DEFAULT_FOLDER = beekeeb/piantor/pico to the existing rules.mk.

@duncanam
Copy link
Author

Basically. You'd have two subfolders named say pico and weact, both with empty rules.mk (to make them build targets), but one also has a config.h with the VBUS pin define. Then you add DEFAULT_FOLDER = beekeeb/piantor/pico to the existing rules.mk.

Got it- that does seem cleaner.

@drashna drashna requested a review from a team August 12, 2023 07:09
Copy link
Contributor

@lesshonor lesshonor left a comment

Choose a reason for hiding this comment

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

Inclined to agree with fauxpark that the top-level configuration shouldn't have USB_VBUS_PIN defined.
The "reference" Pi Pico pin (GP24) would be configured in a raspi subfolder (...I think this might be a better name than pico, since there are so many different "Pi Pico"s) and the WeAct Pi Pico pin (GP29) would be configured in a weact subfolder—e.g.:

piantor/
├── no_vbus_sense/
│   └── rules.mk
├── raspi/
│   ├── config.h
│   └── rules.mk
├── weact/
│   ├── config.h
│   └── rules.mk
├── README.md
├── config.h
└── rules.mk

(The no_vbus_sense folder wouldn't be necessary if the default build didn't need to assume a pin.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# This file intentionally left blank

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# This file intentionally left blank

@drashna drashna self-requested a review October 22, 2023 03:44
Comment on lines +14 to +15
// When USB_VBUS_PIN is not defined, SPLIT_USB_DETECT is used.
#define USB_VBUS_PIN GP24 // for Raspberry Pi Pico
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// When USB_VBUS_PIN is not defined, SPLIT_USB_DETECT is used.
#define USB_VBUS_PIN GP24 // for Raspberry Pi Pico

// SPDX-License-Identifier: GPL-2.0-or-later
#pragma once

#undef USB_VBUS_PIN
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#undef USB_VBUS_PIN

@duncanam
Copy link
Author

duncanam commented Nov 3, 2023

Thanks for the feedback, I'll take a look

Copy link

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 bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

@github-actions github-actions bot added the stale Issues or pull requests that have become inactive without resolution. label Dec 19, 2023
@duncanam
Copy link
Author

Status update: I am slow

@github-actions github-actions bot removed the stale Issues or pull requests that have become inactive without resolution. label Dec 20, 2023
Copy link

github-actions bot commented Feb 3, 2024

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 bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

@github-actions github-actions bot added the stale Issues or pull requests that have become inactive without resolution. label Feb 3, 2024
@duncanam
Copy link
Author

duncanam commented Feb 3, 2024 via email

@github-actions github-actions bot removed the stale Issues or pull requests that have become inactive without resolution. label Feb 4, 2024
Copy link

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 bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

@github-actions github-actions bot added the stale Issues or pull requests that have become inactive without resolution. label Mar 21, 2024
Copy link

Thank you for your contribution!
This pull request has been automatically closed because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, or re-open when it's ready.
// [stale-action-closed]

@github-actions github-actions bot closed this Apr 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keyboard stale Issues or pull requests that have become inactive without resolution.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants