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

Data-driven ChibiOS peripheral configuration #24370

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

tzarc
Copy link
Member

@tzarc tzarc commented Sep 7, 2024

Description

As discussed on Discord. Raised for visibility and further discussion.

Types of Changes

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

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

@tzarc tzarc requested a review from a team September 7, 2024 07:42
@github-actions github-actions bot added core python cli qmk cli command dd Data Driven Changes labels Sep 7, 2024
Comment on lines +16 to +19
FIELD_MAPPING = {
"backlight.pin": "backlight",
"ws2812.pin": "ws2812",
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We would also need to take the corresponding driver property into account somehow. If we want this code to be really generic, we could even add the driver name to the mapping file format, so that instead of {peripheral}.{pin} we would have {peripheral}.{driver}.{pin}. Although even this might not be enough to support the spi driver for ws2812 in a completely generic way, because that driver would also require specifying a real SCK pin in many cases, and maybe even a real MISO pin on old chips like F103 where having a peripheral enabled would interfere with the AF functionality of all its pins.

Also do we want to provide some kind of escape hatch here (disable generation of ChibiOS configs for a particular peripheral)? This may need some new JSON keys though.

Another thing which might need either new JSON keys or some magic conversion for pin names is the selection of the particular chip configuration from multiple ones which could be possible for the same output pin. E.g., once I made a list of all possible WS2812 PWM configurations for STM32F072C chips, and it looks like this:

a0_tim2
a1
a10
a15_tim2
a1_tim2
a2
a2_tim2
a3
a3_tim2
a5_tim2
a6
a6_tim16
a6_tim16_dma1s4
a6_tim16_dma1s6
a7
a7_tim1
a7_tim17
a7_tim17_dma1s2
a7_tim17_dma1s7
a8
a9
b0
b0_tim1
b1
b10_tim2
b11_tim2
b13
b14
b14_tim15
b15
b15_tim15
b15_tim15ch1n
b1_tim1
b3_tim2
b4
b5
b6
b6_tim16_dma1s4
b6_tim16_dma1s6
b7
b7_tim17_dma1s2
b7_tim17_dma1s7
b8
b8_tim16_dma1s4
b8_tim16_dma1s6
b9
b9_tim17_dma1s2
b9_tim17_dma1s7

As you can see, many pins have multiple timer options, and you may need to choose a particular one because of a conflict with other usage of the same timer or the DMA stream for TIMx_UP, and in some cases there are even multiple DMA stream options. And a fully automated selection might not be possible, because the generation code might not know about all usages of timers, DMA streams and any other chip resources (it could detect and try to resolve some conflicts if we also bring the configuration of SPI, I2C, UART, … to the same system, but some way to specify the configuration manually would be wanted anyway).

Copy link
Contributor

Choose a reason for hiding this comment

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

STM32F103C seems to have less alternate options:

a0
a1
a10
a15
a2
a3
a6
a7
a7_tim1
a8
a9
b0
b0_tim1
b1
b10
b11
b13
b14
b15
b1_tim1
b3
b4
b5
b6
b7
b8
b9

But in that case the problem is that for some pins, like A15, you need this:

void board_init(void) {
    // Remap TIM2 from CH1/ETR/PA0,  CH2/PA1
    //              to CH1/ETR/PA15, CH2/PB3
    AFIO->MAPR = (AFIO->MAPR & ~AFIO_MAPR_TIM2_REMAP) | AFIO_MAPR_TIM2_REMAP_PARTIALREMAP1;
    // (another option that would work is AFIO_MAPR_TIM2_REMAP_FULLREMAP, which
    // would also remap CH3/PB10, CH4/PB11)
}

Not sure how important would be the ability to choose a particular remapping configuration in this case. In theory the timer should not affect the functionality of pins for which the corresponding timer channel is not enabled, but there might be some corner cases there; in particular, if we ever add support for using some timer channel instead of TIMx_UP to generate DMA requests (which might be needed to resolve some DMA stream conflicts), using TIM1_CH2 for that purpose can trigger some chip errata.

Comment on lines +57 to +58
"#define WS2812_PWM_DMA_CHANNEL 1",
"#define WS2812_PWM_DMAMUX_ID STM32_DMAMUX1_TIM20_UP"
Copy link
Contributor

Choose a reason for hiding this comment

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

WS2812_PWM_DMA_CHANNEL should not be specified for this chip.

WS2812_PWM_DMA_CHANNEL refers to a form of request selection done by the DMA controller (some chips, like F4x1, have CHSEL bits in the DMA_SxCR register, some others, like F072, have a separate DMA_CSELR register; F103 and F303 don't have such selection and rely on special remapping registers in cases when the same DMA request can be handled by multiple streams, so WS2812_PWM_DMA_CHANNEL should not be defined for those chips too). WS2812_PWM_DMAMUX_ID refers to the request selection done by the separate DMAMUX peripheral; if DMAMUX is present, the first form of request selection is not really needed, and probably is not implemented in any existing chips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli qmk cli command core dd Data Driven Changes python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants