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

Add STM32L432 MCU & EEPROM emulation support #16012

Closed
wants to merge 4 commits into from

Conversation

vinorodrigues
Copy link
Contributor

@vinorodrigues vinorodrigues commented Jan 23, 2022

Description

This PR appends to core, by:

  • Adding support for the STM32L432 MCU
  • Adds EEPROM emulation for STM32L4xx series

I have not marked this as core though, even though the PR Checklist explicitly states that core amendments will need to reside on the develop branch, but if you (QMK admins) would honour me by sacrificing your time to read my beseechment as to why I implore you to merge this into core as hastily as you can muster.

(I'd like to preface this by stating that I do not work for Keychron, nor affiliated with them in any way. I have not even been able to contact them successfully yet. I'm also not a professional developer and I have no desire to develop - but I do love my Q1, and believe this problem is worthy of my passion.)

Keychron, a significant mouse and keyboard peripheral company has recently created a whole range of enthusiast keyboards for the custom keyboard market. They, by all evidence, have fully committed to building their next range of consumer products as boy/cyo platforms that allow both builder and coders to take these products wherever thy want to. But, as I understand it from Discord discussions, they ran into supply issues with the ATMega32's and opted to inmplement the STM32L432 IC (my personal assumption why this one and not one that is more commonly used in probably the pin placement and count in comparison to the ATMega32, but idk.).

Anyway, long story short, they have already released 2 products (the Q1 and the Q2) into the market that have been adopted by the community with somewhat guster, but alas WITHOUT official QMK:master inclusion, all because the STM32L432 is not included in that reference branch. The reason why the Q1 STM32 based keeb is not in the repo. is because the source will not compile without a small handful of non-breaking changes to the core.

Should these changes be merged then both Keychron's developed and, alas, the community at large can begin to contribute to making this an outstanding offering in the QMK community - thereby further strengthening QMK's immense value to the open-source community. I truly believe that QMK has completely changed the landscape of keyboard peripherals, and with that Keychron's all-in approach to this platform should be both applauded and supported.

With that - I will try to articulate the files submitted in this PR -- and you (the QMK admins) are more than welcome to ignore my perpetual ramblings -- even tough I'm not entirely the author there of, with the aim of proving that this is non-breaking to core, but is a severe impediment to Keychron's SMT32 open-source without.

Generally

  • The STM32L432 is not used by any other project currently on QMK:master - addition of this is non-affecting to any other build.
  • The STM32L432 is already present in the ChibiOS library
  • It is functionally (re. API) the same as the STM32L433, that is already supported

common_features.mk

Lines 192-197 add a MCU_SERIES filter on the STM32L4xx value

  • no other keyboard uses this value
  • it is protected from general compile by a ifneq directive

drivers/led/ckled2001.c

Lines 128-137 make a small change to the proprietary Keychron LED driver.

platform/chibios/eeprom_stm32_defs.h

Line 28 and then 30 append to the end of the #elif directive a || defined(STM32L432xx).

  • this is a "me too" statement that enables the FEE_PAGE_SIZE and the FEE_MCU_FLASH_SIZE to be set for that MCU. The "me too"-ness means it will have no effect on the rest of the core.

platform/chibios/eeprom_stm32_l4.h & platform/chibios/eeprom_stm32_l4.c

This is the STM32L432 EEPROM emulator.

These are new files added to the repertoire - from what I can understand of it; it looks to be performing the same work as core's eeprom_stm32.* set, but with the move from BYTE based functions to DWORD based functions.

  • no other keyboard uses this MCU or it's EEPROM emulation methodology

platform/chibios/flash_stm32.h and platform/chibios/flash_stm32.c

Also part of the STM32L432 EEPROM emulator

Line 54-62, 140-142, 155-156 and 201-233 all help to enable the DWORD based writing.

  • all the changes are isolated from other builds with #if defined(STM32L4XX) conditional directives

Final remarks

As you can see - this PR has very little (or no) risks to the core code, but it's inclusion will enable an already great product to really shine. To be forthright here, Keychron already host their own fork of QMK and already host compilable source on their playground branch - but alas, this code base is based on 0.14.29, and I personally don't have the want to compile from a base that is not core ... and I assume many of my cohort QMK enthusiast would agree.

Also worth noting that this code already exists in another PR, #15050, but that PR has unresolved conflicts that are not been addressed (as described in the Issues Fixed section below), but I'm of the opinion that they may be overwhelmed by other matters (idk, guessing here).

Please, please, look at this with a sense of exigency that befits a community of 100's of developers wanting to fiddle with their keyboards.

--made with ♡, Vino Rodrigues

Types of Changes

  • Core (see above)
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Issues Fixed or Closed by This PR

  • This PR is a proposed replacement of PR Add STM32L432 support and STM32L4 series EEPROM emulation #15050, as said PR:
    • is tethered to the develop branch
    • is (as of the time of this pull) 640 commits behind qmk:master
    • leverages ChibiOS folder nested under tmk_core/common, that has since moved to platforms and, as such, will not function in its current state.
    • introduces a whole platforms/chibios/boards/GENERIC_STM32_L432KC folder structure that is actually not needed as the STM32L432 code base compiles the already existing STM32L433 iteration
    • --- besides that, its the exact same code! (... and I wish that there was a means to mark @lokher as the author.)

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

@zvecr zvecr changed the base branch from master to develop January 23, 2022 14:47
@zvecr
Copy link
Member

zvecr commented Jan 23, 2022

15050 isnt really complete at this point, so im not sure a rebase of it is going to land directly to master.

It implements APIs inconsistently, which do not function on every platform. FLASH_ProgramDoubleWord not existing, FLASH_ProgramHalfWord not functioning.

@drashna drashna requested a review from a team January 23, 2022 20:52
@vinorodrigues
Copy link
Contributor Author

I don't get it. After spending hours on this to explain that this code in absolutely critical in supporting the progression of a vendors products who have staked all it's bets on QMK, only to get commentary that circumvents the reasoning that I've tried to articulate.

FLASH_ProgramDoubleWord does exist, and it's implemented in flash_stm32.c, and yes it does indeed not work on every platform, hence the enclosing #if defined(STM32L4XX) (line 201) that isolates it to only this platform (that be STM32L4XX), and also yes FLASH_ProgramHalfWord does not work in this case as it's not implemented in file eeprom_stm32_l4.c that in turn is only included on a STM32L4XX compile.

If however you are saying that QMK core would rather not create platform specific iterations of it's code, then that should have been said instead - and possibly an alternative approach suggested.

I'm in hope that @drashna or @tzarc can cast their eyes on this - and consider my proposition, and possibly offer guidance if it does not align to their thinking - I'm happy to learn from the experience as that is indeed the reason I code as a hobby . Failing that, I will just withdraw the PR as I'm not willing to put dozens of hours i've put of my own personal time on this for a $150 keyboard I can just dump in the trash and move on.

@zvecr
Copy link
Member

zvecr commented Jan 23, 2022

The explanation you have provided isnt the issue. As i mention, the code introduces an inconsistent API, and only capturing its use within the introduced code ignores the contracts we put in place between master and develop. FLASH_ProgramDoubleWord not working on existing platforms, but declared as a public API would be considered breaking. There's enough context here to suggest a few ways forward, but i will mention what initially comes to mind

  • Implement missing APIs
    • A bit of a non starter due to FLASH_ProgramHalfWord on L4
  • Add further ifdefs to protect the various functions and declarations or stub with static asserts
    • Introduces fragmentation

Vendors should plan better, we shouldn't have to change our process to work round them.

Failing that, I will just withdraw the PR as I'm not willing to put dozens of hours i've put of my own personal time on this for a $150 keyboard I can just dump in the trash and move on.

Dont worry, We can do this for you and handle any required changes/retargetting ourselves on the original PR with the original author.

@zvecr zvecr closed this Jan 23, 2022
@tzarc
Copy link
Member

tzarc commented Jan 23, 2022

FWIW I'd already prepped an L432 and L442 branch but hadn't gotten around to submitting it due to IRL stuff.
There were questions regarding the emulated EEPROM, and at this point still need to be reconsidered to a much greater degree.
The other aspect was that there were multiple changes being included at the same time, and for review purposes it's significantly easier (and recommended) if they're split up.

I've submitted my L432/L442 as #16016 to address the missing board, so now the only thing in question is the EEPROM.

@vinorodrigues
Copy link
Contributor Author

vinorodrigues commented Jan 24, 2022

Thanks @tzarc - I get it. This is why I raised this PR in the first place - because I know that the KC staff are not overly familiar with the norms of open-source pulls. That and the knowledge that there will be a significant language barrier in this, lead me to hubristically assume I could help.

Alas your #16016 looks to be the ticket and I PROFOUNDLY thank you for that. I will undertake some testing on this and feedback if I encounter any issues. If you don't hear from then then assume all is good (or that I've given up and walked away from all this). (I may also PR a tiny fix for the LED driver on a seperate PR.)

Again ... thank you, thank you, thank you!

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.

3 participants