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

support a new MCU:FS026. add a new keyboard:essemi/fs026. #23392

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from
Open

support a new MCU:FS026. add a new keyboard:essemi/fs026. #23392

wants to merge 11 commits into from
+519 −0

Conversation

liuhy-2020
Copy link

@liuhy-2020 liuhy-2020 commented Apr 2, 2024 •
edited

support a new MCU:FS026.
add a new keyboard:essemi/fs026.

Need to update ChibiOS-Contrib(Merged PR:ChibiOS/ChibiOS-Contrib#397)

Description

Types of Changes

  • [v] Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • [v] Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Issues Fixed or Closed by This PR

Checklist

  • [v] My code follows the code style of this project: C, Python
  • I have read the PR Checklist document and have made the appropriate changes.
  • [v] 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.
  • [v] I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@github-actions github-actions bot added core keyboard keymap python cli qmk cli command dd Data Driven Changes labels Apr 2, 2024
@zvecr zvecr changed the base branch from master to develop April 2, 2024 01:56
@tzarc
Copy link
Member

tzarc commented Apr 2, 2024

Half of my review comments on the previous PR still apply.
#23233 (review)

@tzarc tzarc marked this pull request as draft April 2, 2024 02:00
@tzarc tzarc added invalid pr_checklist_pending Needs changes as per the PR checklist labels Apr 2, 2024
@tzarc tzarc mentioned this pull request Apr 2, 2024
Closed
14 tasks
@tzarc tzarc added the awaiting_pr Relies on another PR to be merged first label Apr 2, 2024
@tzarc
Copy link
Member

tzarc commented Apr 2, 2024 •
edited

Also, why are things ES32 as well as FS026? Should the MCU actually be called ES32FS026, much like this? Why are you using ES32 if it's been renamed?

@liuhy-2020
Copy link
Author

The MCU library I am using comes from ES32_SDK.
In ES32_SDK, the MCU is referred to as ES32F0283_FS026.
ES32F0283 and FS026 are different MCUs. I can find them all in the chip package. The majority of peripherals used in ES32F0283 and FS026 are the same. But the resources are different.

@liuhy-2020
Copy link
Author

The reason for QMK CI Build failure is that ChibiOS Contrib has not been updated to the latest version.

next step： add a corresponding test board under keyboards/handwired/onekey?

@liuhy-2020
Copy link
Author

@tzarc Is it convenient to update ChibiOS Contrib? (FS026 merged into ChibiOS Contrib last week)

@tzarc tzarc added the no-ci Signals to the CI runners not to build. label Apr 3, 2024
@tzarc
Copy link
Member

tzarc commented Apr 3, 2024 •
edited

CI disabled until we update ChibiOS-Contrib -- #23405.

@tzarc tzarc removed the no-ci Signals to the CI runners not to build. label Apr 5, 2024
@liuhy-2020
Copy link
Author

CI disabled until we update ChibiOS-Contrib -- #23405.

When will the ChibiOS-Contrib be updated approximately?

@tzarc
Copy link
Member

tzarc commented Apr 8, 2024

When will the ChibiOS-Contrib be updated approximately?

Already done.

tzarc
tzarc requested changes Apr 8, 2024
View reviewed changes
Copy link
Member

@tzarc tzarc left a comment

Choose a reason for hiding this comment

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

After everything has been applied, the only changes in this PR should be under keyboards/essemi. No other files need to be modified at this stage.

data/schemas/keyboard.jsonschema Outdated Show resolved Hide resolved
keyboards/essemi/fs026/config.h Outdated Show resolved Hide resolved
keyboards/essemi/fs026/fs026.c Outdated Show resolved Hide resolved
keyboards/essemi/fs026/halconf.h Outdated Show resolved Hide resolved
keyboards/essemi/fs026/halconf.h Outdated Show resolved Hide resolved
keyboards/essemi/fs026/chconf.h Outdated Show resolved Hide resolved
keyboards/essemi/fs026/config.h Outdated Show resolved Hide resolved
keyboards/essemi/fs026/fs026.c Show resolved Hide resolved
keyboards/essemi/fs026/halconf.h Outdated Show resolved Hide resolved
keyboards/essemi/fs026/keymaps/default/keymap.c Outdated Show resolved Hide resolved
@tzarc tzarc removed the awaiting_pr Relies on another PR to be merged first label Apr 8, 2024
@tzarc tzarc requested a review from a team April 8, 2024 06:54
@github-actions github-actions bot removed python cli qmk cli command dd Data Driven Changes labels Apr 9, 2024
@github-actions github-actions bot removed the core label Apr 9, 2024
tzarc
tzarc requested changes Apr 9, 2024
View reviewed changes
Copy link
Member

@tzarc tzarc left a comment

Choose a reason for hiding this comment

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

More changes.

keyboards/essemi/fs026/fs026.c Outdated Show resolved Hide resolved
keyboards/essemi/fs026/rules.mk Outdated Show resolved Hide resolved
keyboards/essemi/fs026/halconf.h Outdated Show resolved Hide resolved
keyboards/essemi/fs026/keyboard.json Show resolved Hide resolved
keyboards/essemi/fs026/mcuconf.h Outdated
Comment on lines 92 to 95
#define ES32_PWM_USE_TIM1 FALSE
#define ES32_PWM_USE_TIM2 FALSE
#define ES32_PWM_USE_TIM3 FALSE
#define ES32_PWM_USE_TIM4 FALSE
Copy link
Member

Choose a reason for hiding this comment

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

These are written as TIM1...TIM4, but the backlight is called GP16C4T2.
The defines don't match https://github.com/ChibiOS/ChibiOS-Contrib/blob/chibios-21.11.x/os/hal/ports/ES32/LLD/TIMv1/hal_pwm_lld.h

Please fix up this file so that ALL item names are actually correct, not just the PWM ones.

keyboards/essemi/fs026/keyboard.json Show resolved Hide resolved
keyboards/essemi/fs026/keymaps/default/keymap.c Show resolved Hide resolved
@tzarc tzarc removed invalid pr_checklist_pending Needs changes as per the PR checklist labels Apr 9, 2024
@tzarc tzarc marked this pull request as ready for review April 9, 2024 10:40
@tzarc tzarc requested a review from a team April 9, 2024 10:40
@liuhy-2020
Copy link
Author

What modifications are needed?

@tzarc
Copy link
Member

tzarc commented Apr 17, 2024

What modifications are needed?

Your mcuconf.h is now in the wrong format and only configures PWM entries, but should be configuring every available peripheral. You also have not addressed the issue with the readme.

@tzarc tzarc mentioned this pull request Apr 17, 2024
Closed
14 tasks
@liuhy-2020
Copy link
Author

If the MCU driver currently only supports GPIO, USB, and PWM functions. Is this okay?

In the future, improve MCU drivers.

@tzarc
Copy link
Member

tzarc commented May 14, 2024

This is what a normal mcuconf.h would look like for a typical Chinese STM32 clone: https://github.com/qmk/qmk_firmware/blob/master/platforms/chibios/boards/GENERIC_WB32_F3G71XX/configs/mcuconf.h

Note that it includes all peripheral types. This is the expectation, even if they're not yet implemented in ChibiOS-Contrib.

I understand you've only implemented the bare minimum in the base layer, but we don't want to be modifying and adapting these files every time you onboard a new peripheral.

As it stands the only peripheral you have added is PWM, so technically the default should be that USB and GPIO are disabled and your keyboard shouldn't work. As such, we'll assume you've misconfigured your code in ChibiOS-Contrib if they're working without being configured. Might be best fixing that up -- peripherals should be disabled by default in the base layer.

@liuhy-2020
Copy link
Author

OK.I will add all peripherals in mcuconf. h.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tzarc tzarc tzarc requested changes

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

Successfully merging this pull request may close these issues.

None yet
2 participants
@liuhy-2020 @tzarc