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

Cradio #224

Merged
merged 24 commits into from Oct 15, 2020
Merged

Cradio #224

merged 24 commits into from Oct 15, 2020

Conversation

davidphilipbarr
Copy link
Contributor

34 key keyboard with no diodes.

Copy link
Contributor

@petejohanson petejohanson left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. A mixture of items from header tweaks to some shield details.

# across A & B controllers, and STM32F303CCT6 can't enable
# interrutps for multiple controllers for the same "line"
# for the external interrupts.
config ZMK_KSCAN_GPIO_POLLING
Copy link
Contributor

Choose a reason for hiding this comment

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

Were you testing with proton-c controllers? If so, there's actually a way to provide board specific overrides for a given shield, you can see that in https://docs.zephyrproject.org/2.3.0/guides/porting/shields.html?highlight=shield#board-specific-shield-configuration

I would remove this from the "base" defconfig here, and put it in the proton-c specific one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not clear what you mean here so i just deleted everything about config...

@@ -0,0 +1,14 @@

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add the standard ZMK header here, e.g.

# Copyright (c) 2020 The ZMK Contributors
# SPDX-License-Identifier: MIT

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

@@ -0,0 +1,5 @@
# Copyright (c) 2020 Pete Johanson
Copy link
Contributor

Choose a reason for hiding this comment

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

We're still working on fixing up existing boards, but please use the "single author file" version like so:

Suggested change
# Copyright (c) 2020 Pete Johanson
# Copyright (c) 2020 The ZMK Contributors

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

Copyright (c) 2020 The ZMK Contributors

SPDX-License-Identifier: MIT

.gitignore Outdated
@@ -4,3 +4,4 @@
/tools
/zephyr
/build
**/.DS_Store
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

This sadly conflicts w/ main now. Can you revert this, then we can merge?

@@ -0,0 +1,5 @@
# Copyright (c) 2020 Pete Johanson
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

Suggested change
# Copyright (c) 2020 Pete Johanson
# Copyright (c) 2020 The ZMK Contributors

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added# Copyright (c) 2020 The ZMK Contributors

SPDX-License-Identifier: MIT


};

&kscan0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be merged with the initial kscan0 definition above this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i've nudged this around and it seem to still compile...

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I wasn't clear, this should just something like:

kscan0: kscan {
  input-gpios = <foo>;
};

Altogether, you don't need to define it further up, then reference it by "anchor" here again.

app/boards/shields/cradio/cradio_left.overlay Outdated Show resolved Hide resolved
app/boards/shields/cradio/cradio.keymap Show resolved Hide resolved
app/boards/shields/cradio/cradio_right.overlay Outdated Show resolved Hide resolved
app/boards/shields/cradio/default.keymap Outdated Show resolved Hide resolved
@innovaker innovaker added the shields PRs and issues related to shields label Oct 2, 2020
@petejohanson petejohanson added the enhancement New feature or request label Oct 5, 2020
Copy link
Contributor

@petejohanson petejohanson left a comment

Choose a reason for hiding this comment

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

A few follow ups, in addition to the few outstanding items. Thanks!

config ZMK_KEYBOARD_NAME
default "cradio"

config ZMK_KSCAN_GPIO_POLLING
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you opened this PR, this got refactored a bit, and the naming improved:

Suggested change
config ZMK_KSCAN_GPIO_POLLING
config ZMK_KSCAN_DIRECT_POLLING

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gotcha, sorted, can't wait to try it.

@@ -0,0 +1,6 @@
/*
* Copyright (c) 2020 Pete Johanson
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a duplicate now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

modded to mirror corne overlay header

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
* Copyright (c) 2020 Pete Johanson
* Copyright (c) 2020 The ZMK Contributors

The Corne overlay header is wrong, and needs fixing, which has been done in another PR that's nearly in.

@@ -0,0 +1,11 @@
/*
* Copyright (c) 2020 Pete Johanson
Copy link
Contributor

Choose a reason for hiding this comment

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

Duplicate here too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

modded to mirror corne overlay header

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
* Copyright (c) 2020 Pete Johanson
* Copyright (c) 2020 The ZMK Contributors

Ditto.

Copy link
Contributor

@petejohanson petejohanson left a comment

Choose a reason for hiding this comment

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

Thanks for the revisions. I few lingering items and then we can get this merged.


};

&kscan0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I wasn't clear, this should just something like:

kscan0: kscan {
  input-gpios = <foo>;
};

Altogether, you don't need to define it further up, then reference it by "anchor" here again.

app/boards/shields/cradio/cradio.keymap Show resolved Hide resolved
@@ -0,0 +1,6 @@
/*
* Copyright (c) 2020 Pete Johanson
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
* Copyright (c) 2020 Pete Johanson
* Copyright (c) 2020 The ZMK Contributors

The Corne overlay header is wrong, and needs fixing, which has been done in another PR that's nearly in.

@@ -0,0 +1,11 @@
/*
* Copyright (c) 2020 Pete Johanson
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
* Copyright (c) 2020 Pete Johanson
* Copyright (c) 2020 The ZMK Contributors

Ditto.

Copy link
Contributor

@petejohanson petejohanson left a comment

Choose a reason for hiding this comment

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

Thanks!

.gitignore Outdated Show resolved Hide resolved
@petejohanson petejohanson merged commit ed28f5a into zmkfirmware:main Oct 15, 2020
1 check passed
MangoIV pushed a commit to MangoIV/zmk that referenced this pull request Dec 18, 2020
tyalie pushed a commit to tyalie/zmk that referenced this pull request Nov 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request shields PRs and issues related to shields
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants