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

boards/common/blxxxpill: Fix pin conflicts in periph_conf #18785

Merged
merged 4 commits into from Oct 27, 2022

Conversation

maribu
Copy link
Member

@maribu maribu commented Oct 21, 2022

Contribution description

  • Move I2C_DEV(0) from PB6/PB7 to PB8/PB9 to solve pin conflict with QDEC_DEV(2).
  • Use pins PB0, PB1, PB4, and PB5 for PWM instead PA8, PA9, PA10, and PA11
    • PA9 and PA10 is in pin conflict with UART_DEV(0) which is used for stdio with stdio_uart, PA8 was in conflict with QDEC_DEV(0), PA11 was in conflict with USB D-
  • Use PB6, PB7 as QDEC_DEV(0) (previously QDEC_DEV(2)), as this is the only completely conflict free setting
  • Use PB4/PB5 instead of PA6/PA7 for QDEC_DEV(1)
    • This fixes a pin conflict with SPI_DEV(0) MISO (and ADC_LINE(4))
    • It now conflicts with PWM, but IMO more use cases needed the main SPI bus than PWM and multiple QDECs. The first QDEC device still is fully conflict free, so SPI + PWM + QDEC_DEV(0) is fine
  • Only provide QDEC at PB4/PB5 when PWM is not used to avoid conflict
  • Only provide QDEC at PA8/PA9 when UART is not used to avoid conflict

The main advantage is now that an application can now use at least the first instance of any peripheral without any conflicts with other peripherals, if I didn't miss anything.

Testing procedure

  • I2C still working (tested with an SHT3x sensor connected via I2C and SAUL, as well as i2c_scan)
  • PWM config (e.g. via tests/periph_pwm and an LED)
  • QDEC config (e.g. via tests/periph_qdec)
I2C test results
2022-10-21 23:51:57,617 # Welcome to RIOT!
2022-10-21 23:51:57,617 # 
2022-10-21 23:51:57,622 # Type `help` for help, type `saul` to see all SAUL devices
2022-10-21 23:51:57,622 # 
> saul read 1
2022-10-21 23:52:04,078 # saul read 1
2022-10-21 23:52:04,082 # Reading from #1 (sht3x1|SENSE_TEMP)
2022-10-21 23:52:04,085 # Data:	         21.78 °C
> 2022-10-21 23:52:04,676 # saul read 1
2022-10-21 23:52:04,681 # Reading from #1 (sht3x1|SENSE_TEMP)
2022-10-21 23:52:04,683 # Data:	         21.78 °C
i2c_scan 0 1
2022-10-21 23:52:07,303 # i2c_scan 0
2022-10-21 23:52:07,305 # Scanning I2C device 0...
2022-10-21 23:52:07,312 # addr not ack'ed = "-", addr ack'ed = "X", addr reserved = "R", error = "E"
2022-10-21 23:52:07,315 #      0 1 2 3 4 5 6 7 8 9 a b c d e f
2022-10-21 23:52:07,318 # 0x00 R R R R R R R R R R R R R R - -
2022-10-21 23:52:07,324 # 0x10 - - - - - - - - - - - - - - - -
2022-10-21 23:52:07,329 # 0x20 - - - - - - - - - - - - - - - -
2022-10-21 23:52:07,334 # 0x30 - - - - - - - - - - - - - - - -
2022-10-21 23:52:07,339 # 0x40 - - - - X - - - - - - - - - - -
2022-10-21 23:52:07,344 # 0x50 - - - - - - - - - - - - - - - -
2022-10-21 23:52:07,348 # 0x60 - - - - - - - - - - - - - - - -
2022-10-21 23:52:07,353 # 0x70 - - - - - - - - R R R R R R R R
> 2022-10-21 23:52:08,117 # i2c_scan 0
2022-10-21 23:52:08,119 # Scanning I2C device 0...
2022-10-21 23:52:08,126 # addr not ack'ed = "-", addr ack'ed = "X", addr reserved = "R", error = "E"
2022-10-21 23:52:08,129 #      0 1 2 3 4 5 6 7 8 9 a b c d e f
2022-10-21 23:52:08,132 # 0x00 R R R R R R R R R R R R R R - -
2022-10-21 23:52:08,137 # 0x10 - - - - - - - - - - - - - - - -
2022-10-21 23:52:08,142 # 0x20 - - - - - - - - - - - - - - - -
2022-10-21 23:52:08,147 # 0x30 - - - - - - - - - - - - - - - -
2022-10-21 23:52:08,152 # 0x40 - - - - X - - - - - - - - - - -
2022-10-21 23:52:08,157 # 0x50 - - - - - - - - - - - - - - - -
2022-10-21 23:52:08,162 # 0x60 - - - - - - - - - - - - - - - -
2022-10-21 23:52:08,167 # 0x70 - - - - - - - - R R R R R R R R
> 2022-10-21 23:52:08,619 # i2c_scan 0
2022-10-21 23:52:08,621 # Scanning I2C device 0...
2022-10-21 23:52:08,627 # addr not ack'ed = "-", addr ack'ed = "X", addr reserved = "R", error = "E"
2022-10-21 23:52:08,631 #      0 1 2 3 4 5 6 7 8 9 a b c d e f
2022-10-21 23:52:08,634 # 0x00 R R R R R R R R R R R R R R - -
2022-10-21 23:52:08,639 # 0x10 - - - - - - - - - - - - - - - -
2022-10-21 23:52:08,645 # 0x20 - - - - - - - - - - - - - - - -
2022-10-21 23:52:08,649 # 0x30 - - - - - - - - - - - - - - - -
2022-10-21 23:52:08,655 # 0x40 - - - - X - - - - - - - - - - -
2022-10-21 23:52:08,659 # 0x50 - - - - - - - - - - - - - - - -
2022-10-21 23:52:08,664 # 0x60 - - - - - - - - - - - - - - - -
2022-10-21 23:52:08,669 # 0x70 - - - - - - - - R R R R R R R R
### Issues/PRs references

None

@maribu maribu added Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Area: boards Area: Board ports labels Oct 21, 2022
@maribu maribu force-pushed the boards/blxxxpill/periph_conf branch from ac9f595 to f3f1cf9 Compare October 21, 2022 22:07
@github-actions github-actions bot added the Area: examples Area: Example Applications label Oct 21, 2022
@maribu
Copy link
Member Author

maribu commented Oct 21, 2022

This is definitely breaking qdec :/

@maribu maribu force-pushed the boards/blxxxpill/periph_conf branch from f3f1cf9 to 8650e8d Compare October 21, 2022 23:45
@github-actions github-actions bot added Area: cpu Area: CPU/MCU ports Platform: ARM Platform: This PR/issue effects ARM-based platforms labels Oct 21, 2022
Comment on lines 147 to 168
#ifndef MODULE_PERIPH_PWM
{
.dev = TIM3,
.max = 0x0000ffff,
.rcc_mask = RCC_APB1ENR_TIM3EN,
.chan = { { .pin = GPIO_PIN(PORT_B, 4), .cc_chan = 0 },
{ .pin = GPIO_PIN(PORT_B, 5), .cc_chan = 1 } },
.bus = APB1,
.irqn = TIM3_IRQn,
/* be default TIM3 is routed to PA6 (cc_chan 0) and PA7 (cc_chan 1) */
.remap = AFIO_MAPR_TIM3_REMAP_1,
},
#endif
Copy link
Member Author

Choose a reason for hiding this comment

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

This one now works with support for remapping implemented. Still, the QDEC for TIM4 doesn't work. I think it never worked... I wonder how it got upstreamed.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, this took me way longer to figure out than I had hoped for: The STM32F103xxx bluepill I used has no TIM4. With my Blackpill, the TIM4 (PB6 / PB7) QDEC config works just fine.

@maribu maribu force-pushed the boards/blxxxpill/periph_conf branch 2 times, most recently from a4b4a7c to 3c011c4 Compare October 22, 2022 00:51
@github-actions github-actions bot removed the Area: examples Area: Example Applications label Oct 22, 2022
@gschorcht
Copy link
Contributor

ADC channels

    { .pin = GPIO_PIN(PORT_A, 4), .dev = 0, .chan = 4 },
    { .pin = GPIO_PIN(PORT_A, 5), .dev = 0, .chan = 5 },
    { .pin = GPIO_PIN(PORT_A, 6), .dev = 0, .chan = 6 },
    { .pin = GPIO_PIN(PORT_A, 7), .dev = 0, .chan = 7 },

are still in conflict with SPI_DEV(0), right? Would it make sense to use SPI2 instead of SPI1 for SPI_DEV(0) and to configure SPI1 as SPI_DEV(1) only if periph/adc isn't used?

ADC channels

    { .pin = GPIO_PIN(PORT_B, 0), .dev = 0, .chan = 8 },
    { .pin = GPIO_PIN(PORT_B, 1), .dev = 0, .chan = 9 },

are in conflict with PWM_DEV(0) channel 2 and 3, right?

@maribu
Copy link
Member Author

maribu commented Oct 23, 2022

Indeed. Sorry for being lazy here and only fixing the issue related to my use case. I should have done this properly in the first place. I will do so now.

@maribu maribu force-pushed the boards/blxxxpill/periph_conf branch from 3c011c4 to 6d6339c Compare October 23, 2022 21:36
@maribu maribu requested a review from jia200x as a code owner October 23, 2022 21:36
@github-actions github-actions bot added the Area: doc Area: Documentation label Oct 23, 2022
@maribu
Copy link
Member Author

maribu commented Oct 23, 2022

OK, done as suggested. I favored PWM over ADC for PB0 and PB1 (as this results in 6 external ADC pins + 4 PWM pins instead of 8 ADC pins and 2 PWM pins), but when PWM is unused all 8 pins of the Bluepill remain available.

Ideally these prefer one over the other decisions should be configurable. But I'd prefer to wait until the KConfig migration is done to only model this once.

I added a pinout generated using https://github.com/stevenj/GenPinoutSVG and the source CSV file it was generated from. That pinout contains the peripheral configuration of the board provided this PR. (Compared to other pinouts, it uses the RIOT numbering and omits peripheral support not configured in RIOT, making the pinout much more handy for RIOT users and much less handy for everybody else.)

I had to apply the following patch to get the tool running, though:

Update: Fixed upstream :)

diff --git a/GenPinoutSVG.py b/GenPinoutSVG.py
index a4e7912..f2beae7 100755
--- a/GenPinoutSVG.py
+++ b/GenPinoutSVG.py
@@ -224,9 +224,10 @@ def writeIcon(params):
       return False
 
     # Then get raw PNG data and encode DIRECTLY into the SVG file.
-    image_data = img.make_blob()
-    encoded = base64.b64encode(image_data).decode()
-    pngdata = 'data:image/svg+xml;base64,{}'.format(encoded)
+    with open(imgfile, "rb") as imgfile:
+      image_data = imgfile.read()
+      encoded = base64.b64encode(image_data).decode()
+      pngdata = 'data:image/svg+xml;base64,{}'.format(encoded)
 
     W = round(get_size(W,img.width))
     H = round(get_size(H,img.height))

I am not entirely happy with that tool. I would have favored something that is more descriptive and splits out the styling. But I couldn't find anything better quickly.

@kfessel
Copy link
Contributor

kfessel commented Oct 25, 2022

Wouldn't this change be a new board?

This likely breakes user applications that use the old board definition.
Is that something that no one should have done (is there a statement like "if you use riot and attach something to a board please copy the board and make it your own" and we consider board-definitions sane defaults that may change breaking)

@maribu
Copy link
Member Author

maribu commented Oct 25, 2022

RIOT has a history of regularly changing board definitions. If you attach hardware permanently to your board (e.g. by adding a shield), application developers will naturally create a copy of that board and extend the board.h as needed. This is just because it is much easier to set up the configuration once in an folder listed in EXTERNAL_BOARD_DIRS, rather than having to copy-paste CFLAGS += -DPARAM_FOO_BAR=xy and the default saul / netdev drivers into the Makefile of every application. As a side effect this will prevent upstream changes to a board definition to render a working application defunct.

In this case, it fixes bugs:

  • Currently, using PWM together with USB will break both PWM and your USB connecting.
  • Currently, using QDEC together with UART will break the UART connection that is used for STDIO by default for the non-128-KiB versions. The test application for QDEC justs goes silent after it configures the QDEC devices. And I doubt that QDEC keeps functioning when the pins are used for stdio as well.
  • Currently, using ADC together with SPI breaks your SPI bus

Quite generally, the STM32F1 routes peripherals to pins, not pins to peripheral (like all other STM32s). You can route two peripherals to the same pin. I have not checked what happens when two peripherals try to drive the exact same pin to opposite logic levels. But the current config of the Bluepill board should have never been merged. The issue seems that only the added config has been tested in isolation. E.g. ADC without SPI works fine.

@maribu maribu added the Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) label Oct 25, 2022
@maribu maribu changed the title boards/common/blxxxpill: Improve pin conflicts in periph_conf boards/common/blxxxpill: Fix pin conflicts in periph_conf Oct 25, 2022
@gschorcht
Copy link
Contributor

@maribu Please squash.

@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 26, 2022
@riot-ci
Copy link

riot-ci commented Oct 26, 2022

Murdock results

✔️ PASSED

051a1f1 boards/common/blxxxpill: rework periph configuration

Success Failures Total Runtime
1992 0 1992 06m:46s

Artifacts

This only reflects a subset of all builds from https://ci-prod.riot-os.org. Please refer to https://ci.riot-os.org for a complete build for now.

@benpicco
Copy link
Contributor

C++ is not happy

This allows including C headers from C++. It sadly reduced the
diagnostics on C++ code as well, were there warning may make sense as
unintended side effect. We may be able to drop that later on, when more
C APIs are properly wrapped in native C++ APIs, so that C headers do no
longer need to be compatible with C++ compilers.
Add support to route peripheral to alternative pins for the STM32F1
family.
Add support to route timer peripheral to alternative pins for the
STM32F1.
The peripheral configuration has been completely reworked to resolve
pin conflicts while provided as much of the peripherals as possible.
The changes include:

- Move `I2C_DEV(0)` from PB6/PB7 to PB8/PB9 to solve pin conflict with
  `QDEC_DEV(2)`.
- Use pins PB0, PB1, PB4, and PB5 for PWM instead PA8, PA9, PA10, and
  PA11
    - PA9 and PA10 is in pin conflict with `UART_DEV(0)` which is used
      for stdio with `stdio_uart`, PA8 was in conflict with
      `QDEC_DEV(0)`, PA11 was in conflict with USB D-
- Use PB6, PB7 as `QDEC_DEV(0)` (previously `QDEC_DEV(2)`), as this is
  the only completely conflict free setting
- Use PB4/PB5 instead of PA6/PA7 for QDEC_DEV(1)
    - This fixes a pin conflict with `SPI_DEV(0)` MISO (and
      `ADC_LINE(4)`)
- Only provide QDEC at PB4/PB5 when PWM is not used to avoid conflict
- Only provide QDEC at PA8/PA9 when UART is not used to avoid conflict
- Use SPI2 (PB15, PB14, PB13, PB12) as `SPI_DEV(0)` instead of SPI1,
  use SPI1 (PA7, PA6, PA5, PA4) as `SPI_DEV(1)`
- Only provide `SPI_DEV(1)` if the ADC is not in used to resolve a
  pin conflict
- Move PB0 and PB1 at the end of the ADC lines (previously
  `ADC_LINE(6)` and `ADC_LINE(7)`, now `ADC_LINE(8)` and `ADC_LINE(9)`)
   - Only provide them when PWM is not in use (to resolve pin conflict
     with PWM)
   - Also do not provide them for the Blackpill boards, which are
     missing pins PB0 and PB1 on the headers

To make life of users easier, a Pinout diagram with the new
configuration was added.
@maribu maribu force-pushed the boards/blxxxpill/periph_conf branch from 3f0403d to 051a1f1 Compare October 27, 2022 12:29
@github-actions github-actions bot added Area: build system Area: Build system Area: LoRa Area: LoRa radio support Area: pkg Area: External package ports Area: sys Area: System Area: tests Area: tests and testing framework Area: timers Area: timer subsystems labels Oct 27, 2022
@maribu
Copy link
Member Author

maribu commented Oct 27, 2022

All green now.

Note that while I did reorder the initializer list to match the declaration list to please C++ compilers, I didn't add bogus .remap = 0 to all PWM / QDEC configuration but rather added -Wno-missing-field-initializers to CXXFLAGS. Given the number of places that already contained -Wno-missing-field-initializers (and now are dropped, as they are no longer needed), I don't think this is controversial. Still, I do not dare to hit merge myself after such a change post-ACK.

@benpicco benpicco merged commit ed1d8e0 into RIOT-OS:master Oct 27, 2022
@maribu maribu deleted the boards/blxxxpill/periph_conf branch October 27, 2022 19:48
@maribu
Copy link
Member Author

maribu commented Oct 27, 2022

thx :)

@kaspar030 kaspar030 added this to the Release 2023.01 milestone Jan 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: boards Area: Board ports Area: build system Area: Build system Area: cpu Area: CPU/MCU ports Area: doc Area: Documentation Area: LoRa Area: LoRa radio support Area: pkg Area: External package ports Area: sys Area: System Area: tests Area: tests and testing framework Area: timers Area: timer subsystems CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants