-
-
Notifications
You must be signed in to change notification settings - Fork 39.8k
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
[Controller] Added board config for custom controller STeMCell #16287
Conversation
@megamind4089 Can you rebase against the latest breaking change to resolve the conflicts? |
@filterpaper Done. Rebased against the develop branch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and confirmed that these changes work correctly on the current develop
branch with STeMCell hardware version 1.0.1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do the board definitions need a copy of their own board.(c|h)
files? Can they piggyback off existing ones in ChibiOS?
Ideally we should be attempting to leverage as much existing stuff in ChibiOS as possible, providing overrides -- this ensures we've got version independence and allows simpler upgrades for new hardware support. I've got a strong preference to removing these and overriding if we can possibly do so.
#define STM32_PLLM_VALUE 8 | ||
#define STM32_PLLN_VALUE 336 | ||
#define STM32_PLLP_VALUE 4 | ||
#define STM32_PLLQ_VALUE 7 | ||
#define STM32_HPRE STM32_HPRE_DIV1 | ||
#define STM32_PPRE1 STM32_PPRE1_DIV4 | ||
#define STM32_PPRE2 STM32_PPRE2_DIV2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These result in a 21MHz APB1 clock and a 42MHz APB2 clock.
Unsure if there was a specific rationale for this, but for max speed what about:
#define STM32_PLLM_VALUE 8 | |
#define STM32_PLLN_VALUE 336 | |
#define STM32_PLLP_VALUE 4 | |
#define STM32_PLLQ_VALUE 7 | |
#define STM32_HPRE STM32_HPRE_DIV1 | |
#define STM32_PPRE1 STM32_PPRE1_DIV4 | |
#define STM32_PPRE2 STM32_PPRE2_DIV2 | |
#define STM32_PLLM_VALUE 4 | |
#define STM32_PLLN_VALUE 96 | |
#define STM32_PLLP_VALUE 2 | |
#define STM32_PLLQ_VALUE 4 | |
#define STM32_HPRE STM32_HPRE_DIV1 | |
#define STM32_PPRE1 STM32_PPRE1_DIV2 | |
#define STM32_PPRE2 STM32_PPRE2_DIV1 |
This gets APB1 to 48MHz and APB2 to 96MHz.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have reverted the old clock settings to make sure firmware runs in both F401 and F411 chips since f411 is unobtanium now
MEMORY | ||
{ | ||
flash0 (rx) : org = 0x08000000, len = 32k /* Sector 0,1 - TinyUF2 bootloader */ | ||
flash1 (rx) : org = 0x08008000, len = 32k /* Sector 2,3 - Emulated eeprom */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately the way the emulated EEPROM is written, it's going to be situated at the 16kB boundary instead of 32kB.
This makes it difficult to support tinyuf2, given that the bootloader binary is 14kB already and doesn't leave us with a lot of wiggle room.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pardon me, I did not understand this clearly.
tinyuf2 binary size is around 28 Kb and tinyuf2 linker file uses 32kb (first two sectors)
https://github.com/adafruit/tinyuf2/blob/master/ports/stm32f4/linker/stm32f4.ld#L47
I tried to use the next two sectors of each 16KB for EEPROM.
So I defined the EEPROM_BASE_ADDRESS here appropriately:
https://github.com/qmk/qmk_firmware/pull/16287/files#diff-acf2e3a1560d0c323e6c1fce4f5625c68242220a6968c69f175e1df985d1b03eR106
So actual qmk code start from Sector 4(5th sector). Do you foresee any issues with this memory arrangement ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I stated, the way that the code is written is that it's hard-coded to use the 2nd 16k page:
qmk_firmware/platforms/chibios/eeprom_stm32_defs.h
Lines 64 to 66 in edd1f33
# ifndef FEE_PAGE_BASE_ADDRESS | |
# define FEE_PAGE_BASE_ADDRESS 0x08004000 // bodge to force 2nd 16k page | |
# endif |
Defining the location in only the ldscript will not change the implementation using the second page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above code uses the hard coded value, only if users does not define their own FEE_PAGE_BASE_ADDRESS
And i defined the FEE_PAGE_BASE_ADDRESS manually here
https://github.com/qmk/qmk_firmware/pull/16287/files#diff-acf2e3a1560d0c323e6c1fce4f5625c68242220a6968c69f175e1df985d1b03eR106
Is it not sufficient? I have tested the EEPROM config with via and rgb and did not see any issues with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, #16586 also attempts to enable EEPROM emulation together with tinyuf2; maybe the default value of FEE_PAGE_BASE_ADDRESS
should depend on the bootloader. Changing FEE_PAGE_BASE_ADDRESS
for the default case (DFU bootloader in system memory) is probably not desired, because it wastes some flash and increases the apparent firmware size (QMK does not use the DfuSe format extensions which would allow the firmware to be sparse, therefore the whole area reserved for EEPROM and whatever else would need to be flashed together with the rest of the firmware).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed the hard coded FEE_PAGE_BASE_ADDRESS
and moved it to use WEAR_LEVELING_DRIVER with legacy backend.
BOARD := STEMCELL | ||
BOOTLOADER := tinyuf2 | ||
OPT_DEFS += -DCONVERT_TO_STEMCELL | ||
MCU_LDSCRIPT := STEMCELL_tinyuf2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it always require tinyuf2? Are they coming with it preloaded? Do people need to flash it as a bootloader first?
Given that DFU isn't problematic for F4xx, should the default still be DFU, with UF2 as an option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, STeMCell does not have fancy circuit for reset/bootloader logic.
The reset
pinout is directly connected to STM32 NRST pin and Boot0 pin is connected to onboard switch.
With this setup, the normal reset button in keyboard, can only reset. To go to DFU, one has to click Boot0 and then reset button.
Since most of the keyboards will have controllers face down or have OLEDs on top, will be inconvenient to use STM32 inbuilt DFU
TinyUF2 as default, will remove this nuisance and regular user can still single click for reset and double click for bootloader just like promicro
STeMCell after PCBA does not have tiny-uf2 bootloader, We need to load the tinyuf2 bootloader from below using STM32 DFU bootloader.
https://github.com/megamind4089/STeMCell/tree/main/bootloader
@@ -183,7 +183,7 @@ else | |||
ifeq ($(PLATFORM),AVR) | |||
# Automatically provided by avr-libc, nothing required | |||
else ifeq ($(PLATFORM),CHIBIOS) | |||
ifneq ($(filter STM32F3xx_% STM32F1xx_% %_STM32F401xC %_STM32F401xE %_STM32F405xG %_STM32F411xE %_STM32F072xB %_STM32F042x6 %_GD32VF103xB %_GD32VF103x8, $(MCU_SERIES)_$(MCU_LDSCRIPT)),) | |||
ifneq ($(filter STM32F3xx_% STM32F1xx_% STM32F4xx_% %_STM32F401xC %_STM32F401xE %_STM32F405xG %_STM32F411xE %_STM32F072xB %_STM32F042x6 %_GD32VF103xB %_GD32VF103x8, $(MCU_SERIES)_$(MCU_LDSCRIPT)),) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed this changes with my last rebase.
Also i find see other checks for mcus below as redundant, since all of these mcus define MCU_SERIES as STM32F4xx
_STM32F401xC %_STM32F401xE %_STM32F405xG %_STM32F411xE
Shall i change remove the above checks and keep only STM32F4xx_%
?
Or should i use my ld script value to enable EEPROM_DRIVER
I may be missing something here. Please advise
Initially i tried to use the same board config files as black pill which is But I faced an issue when using existing boards board.c/h files, which sigprof helped me to resolve. https://github.com/qmk/ChibiOS/blob/master/os/hal/boards/ST_STM32F401C_DISCOVERY/board.h#L91 Due to this, I2C was not working, since there are two pins mapped to same I2Cs SDA. The board.c/h are tweaked for the particular discovery board and its not a generic one. |
Whilst that would be fine in a world where there's infinite resources to maintain software, the simplest solution is to not have the code in QMK in the first place. The intent behind the removal of board files is to gain version-independence from ChibiOS. This is why keyboards have configuration override includes rather than full config includes, and why there's a very strong preference to using the board definitions already maintained as part of ChibiOS itself. Before any of the above refactoring there were more than 650 board definition and configuration files that needed modifications in order to upgrade ChibiOS -- now we're down to fewer than 30. If we'd ignored the refactoring, we'd probably still be stuck with ChibiOS 17.x, and any potential upgrade would now require modifications of well in excess of 1200 files. So yeah, from a maintenance perspective, there's a very strong preference to leveraging the upstream-maintained copies. |
@tzarc Yes, I understand the concern now and I agree with that. |
Tested and confirmed that it works correctly with the 1.0.1 STeMCell. Currently running on my main build. Thanks, @megamind4089 ! The PR does need to be updated after the recent changes pertaining to |
Confirmed STeMCell v1.0.1 worked with reviung41, crkbd. Thank you, @megamind4089! |
Thank you for your contribution! |
I have a lot of people who are using the stemcell on fingerpunch keyboards at this point, so I'm very interested in what it would take to get this into develop at this point. I am by no means a QMK expert, but if there is anything I can do to help, I'd be more than happy to. |
A similar daughterboard, the bonsai-c4, has just had a converter merged into develop; #17711 |
Tested on a couple of fingerpunch boards (Faux Fox Keyboard, Rock On). No issues after using for a couple of hours. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have also been testing this on my stemcell board, no issues.
Test and confirmed OLED and RGB functionality on CRKBD. |
docs/feature_converters.md
Outdated
The default version selected is v1.0.1. To compile for v1.0.0, use the following flag while compiling. | ||
|
||
```make | ||
-e STMC_VERSION=1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a fan of these compile time flags, feels a lot like the previous CTPC short hand which was removed. (Though im not sure right now on how best to handle the case)
I also have some concerns on versioning being implemented as defines, but thats going to take some time to increment within the current framework.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have any suggestion on how to improve this ?
Or shall I remove the old version to support only current version for now ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the pin swap was to better accommodate SPI pinout, correct? Normally when an external interface changes in an incompatible way (like for a pin swap) you would increment the major version number (e.g. 1.0.0 -> 2.0.0) to signify that backwards compatibility is broken.
Since that isn't what happened here, I would lean towards supporting only the current version in the QMK official repo, yet also providing a separate branch in your QMK fork to support anyone using the older version of the board. As it currently stands, anyone using the old version board has already had to make do without the converter being in the QMK repo, so basically, nothing much would change for them: they can just keep on using a branch in your fork that is dedicated to the older version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, make sense, Also I guess most people have 1.0.1/1.0.2 version.
So i will remove the code supporting older version 1.0.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll have to think up a solution w.r.t. the compile-time flags... but for now it shouldn't hold this PR up.
Has some merge conflicts. |
Thanks @drashna Fixed it. |
…6287) Co-authored-by: Mariappan Ramasamy <947300+Mariappan@users.noreply.github.com> Co-authored-by: Mariappan Ramasamy <maari@basis-ai.com> Co-authored-by: Sadek Baroudi <sadekbaroudi@gmail.com>
Description
This PR adds support for the open source controller STeMCell based on STM32F411 mcu, designed by myself.
STeMCell
Tested the controller with ADux, Lily58 and Swoop keyboard
Types of Changes
Issues Fixed or Closed by This PR
Checklist