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.

Sign up for GitHub

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

Jump to bottom

feat: preonic keyboard #1575

Merged
merged 8 commits into from Dec 16, 2022
Merged

feat: preonic keyboard #1575

merged 8 commits into from Dec 16, 2022

Conversation

jeromeOlivier
Copy link
Contributor

@jeromeOlivier jeromeOlivier commented Dec 9, 2022 •
edited

Board/Shield Check-list

  • This board/shield is tested working on real hardware
  • Definitions follow the general style of other shields/boards upstream (Reference)
  • .zmk.yml metadata file added
  • Proper Copyright + License headers added to applicable files (Generally, we stick to "The ZMK Contributors" for copyrights to help avoid churn when files get edited)
  • General consistent formatting of DeviceTree files
  • Keymaps do not use deprecated key defines (Check using the upgrader tool)
  • &pro_micro used in favor of &pro_micro_d/a if applicable
  • If split, no name added for the right/peripheral half
  • Kconfig.defconfig file correctly wraps all configuration in conditional on the shield symbol
  • .conf file has optional extra features commented out
  • Keyboard/PCB is part of a shipped group buy or is generally available in stock to purchase (OSH/personal projects without general availability should create a zmk-config repo instead)

petejohanson
petejohanson requested changes Dec 9, 2022
View reviewed changes
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 couple high level notes. Can please also check any of the check marks from the PR template that you've done/verified?

app/boards/arm/preonic/pinmux.c Outdated
#endif /* CONFIG_CAN_1 */
};

static int pinmux_stm32_init(struct device *port) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are any of these pinmux bits actually used yet? If not, I'd say please skip this piece altogether, and if/when work happens on things like RGB, that requires SPI, then it can get added then, preferably using the newer pinctrl Zephyr approach instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

file deleted

app/boards/arm/preonic/CMakeLists.txt Outdated
@@ -0,0 +1,13 @@
#
# Copyright (c) 2020 The ZMK Contributors
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update all the copyright dates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

Copy link
Contributor

Choose a reason for hiding this comment

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

Somehow your changes didn't show here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, hadn't pushed. should be there now

@jeromeOlivier jeromeOlivier marked this pull request as draft December 11, 2022 08:29
@jeromeOlivier jeromeOlivier marked this pull request as ready for review December 11, 2022 09:10
petejohanson
petejohanson requested changes Dec 11, 2022
View reviewed changes
app/boards/arm/preonic/Kconfig.defconfig Outdated
Comment on lines 14 to 15
config ZMK_KSCAN_MATRIX_POLLING
default y
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is fully required for this to function, it should actually get set in the _defconfig file, which should force it to stay on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CONFIG_ZMK_KSCAN_MATRIX_POLLING=y already exists onpreonic_rev3_defconfig file, but removing it from Kconfig.defconfig

app/boards/arm/preonic/Kconfig.defconfig Outdated
Comment on lines 11 to 12
config ZMK_USB
default y
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto, there's really no reason not to enable USB for an onboard keyboard, so move to _defconfig file as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood. Will look at it tonight. Many thanks for the notes :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

petejohanson
petejohanson approved these changes Dec 11, 2022
View reviewed changes
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!

@petejohanson
Copy link
Contributor

Looks like the build is failing now though...needs fixing before merging.

@petejohanson
Copy link
Contributor

The actual error when building:

FAILED: zephyr/zephyr_pre0.elf zephyr/zephyr_pre0.map /home/peter/git/zmk/app/build/preonic/zephyr/zephyr_pre0.map 
: && ccache /home/peter/.local/zephyr-sdk-0.15.1/arm-zephyr-eabi/bin/arm-zephyr-eabi-gcc   zephyr/CMakeFiles/zephyr_pre0.dir/misc/empty_file.c.obj -o zephyr/zephyr_pre0.elf  zephyr/CMakeFiles/offsets.dir/./arch/arm/core/offsets/offsets.c.obj  -fuse-ld=bfd  -Wl,-T  zephyr/linker_zephyr_pre0.cmd  -Wl,-Map=/home/peter/git/zmk/app/build/preonic/zephyr/zephyr_pre0.map  -Wl,--whole-archive  app/libapp.a  zephyr/libzephyr.a  zephyr/arch/common/libarch__common.a  zephyr/arch/arch/arm/core/aarch32/libarch__arm__core__aarch32.a  zephyr/arch/arch/arm/core/aarch32/cortex_m/libarch__arm__core__aarch32__cortex_m.a  zephyr/lib/libc/minimal/liblib__libc__minimal.a  zephyr/lib/posix/liblib__posix.a  zephyr/subsys/usb/class/hid/libsubsys__usb__class__hid.a  zephyr/drivers/interrupt_controller/libdrivers__interrupt_controller.a  zephyr/drivers/usb/device/libdrivers__usb__device.a  zephyr/drivers/clock_control/libdrivers__clock_control.a  zephyr/drivers/gpio/libdrivers__gpio.a  zephyr/drivers/hwinfo/libdrivers__hwinfo.a  zephyr/drivers/timer/libdrivers__timer.a  modules/stm32/stm32cube/lib..__modules__hal__stm32__stm32cube.a  modules/drivers/kscan/libzmk__drivers__kscan.a  -Wl,--no-whole-archive  zephyr/kernel/libkernel.a  -L"/home/peter/.local/zephyr-sdk-0.15.1/arm-zephyr-eabi/bin/../lib/gcc/arm-zephyr-eabi/12.1.0/thumb/v7e-m/nofp"  -L/home/peter/git/zmk/app/build/preonic/zephyr  -lgcc  zephyr/arch/common/libisr_tables.a  -no-pie  -mcpu=cortex-m4  -mthumb  -mabi=aapcs  -mfp16-format=ieee  -Wl,--gc-sections  -Wl,--build-id=none  -Wl,--sort-common=descending  -Wl,--sort-section=alignment  -Wl,-u,_OffsetAbsSyms  -Wl,-u,_ConfigAbsSyms  -nostdlib  -static  -Wl,-X  -Wl,-N  -Wl,--orphan-handling=warn && cd /home/peter/git/zmk/app/build/preonic/zephyr && /usr/bin/cmake -E echo
/home/peter/.local/zephyr-sdk-0.15.1/arm-zephyr-eabi/bin/../lib/gcc/arm-zephyr-eabi/12.1.0/../../../../arm-zephyr-eabi/bin/ld.bfd: zephyr/drivers/usb/device/libdrivers__usb__device.a(usb_dc_stm32.c.obj): in function `pinctrl_apply_state':
/home/peter/git/zmk/zephyr/include/drivers/pinctrl.h:345: undefined reference to `pinctrl_lookup_state'
/home/peter/.local/zephyr-sdk-0.15.1/arm-zephyr-eabi/bin/../lib/gcc/arm-zephyr-eabi/12.1.0/../../../../arm-zephyr-eabi/bin/ld.bfd: zephyr/drivers/usb/device/libdrivers__usb__device.a(usb_dc_stm32.c.obj): in function `pinctrl_apply_state_direct':
/home/peter/git/zmk/zephyr/include/drivers/pinctrl.h:326: undefined reference to `pinctrl_configure_pins'
collect2: error: ld returned 1 exit status
ninja: build stopped: subcommand failed.
FATAL ERROR: command exited with status 1: /usr/bin/cmake --build /home/peter/git/zmk/app/build/preonic

You need to add CONFIG_PINCTRL=y to the preonic_rev3_defconfig file and remove the CONFIG_PINMUX=y line.

Then update the USB section of preonic_rev3.dts:

&usb {
	pinctrl-0 = <&usb_dm_pa11 &usb_dp_pa12>;
	pinctrl-names = "default";
	status = "okay";
	cdc_acm_uart: cdc_acm_uart {
		compatible = "zephyr,cdc-acm-uart";
		label = "CDC_ACM_0";
	};
};

Assuming those are the right USB pins used on the preonic as well.

petejohanson
petejohanson requested changes Dec 14, 2022
View reviewed changes
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.

One last thing needed, at least, to get this building.

app/boards/arm/preonic/preonic_rev3.dts Show resolved Hide resolved
petejohanson
petejohanson approved these changes Dec 16, 2022
View reviewed changes
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!

@petejohanson petejohanson merged commit 984b16e into zmkfirmware:main Dec 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petejohanson petejohanson petejohanson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@jeromeOlivier @petejohanson