Skip to content

boards/stm32: introduce and use shared i2c config with I2C1 on PB6/PB7#12141

Merged
benpicco merged 2 commits into
RIOT-OS:masterfrom
aabadie:pr/boards/stm32_common_i2c
Sep 9, 2019
Merged

boards/stm32: introduce and use shared i2c config with I2C1 on PB6/PB7#12141
benpicco merged 2 commits into
RIOT-OS:masterfrom
aabadie:pr/boards/stm32_common_i2c

Conversation

@aabadie

@aabadie aabadie commented Sep 2, 2019

Copy link
Copy Markdown
Contributor

Contribution description

Following #12025 and #12068, this PR is factorizing the configuration for those boards since they are very similar.
iotlab-m3 could also use the new common file but that would first require to provide a shared clock configuration.

@mrusme has the hardware so maybe he could test and report his results here.

Testing procedure

Test an I2C device on nucleo-l031k6 and nucleo-l432kc

Issues/PRs references

Factorization of configurations introduced in #12025 and #12068.

This configuration corresponds to I2C1 connected to PB6 (SCL) and PB7 (SDA)
@aabadie aabadie added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: boards Area: Board ports labels Sep 2, 2019
@aabadie aabadie requested a review from benpicco September 2, 2019 06:18
@benpicco

benpicco commented Sep 2, 2019

Copy link
Copy Markdown
Contributor

Is this really an improvement with the need for those #ifdefs?
Those board configs also serve as an example for new boards so making them more convoluted makes that just harder.

@aabadie

aabadie commented Sep 2, 2019

Copy link
Copy Markdown
Contributor Author

Is this really an improvement with the need for those #ifdefs?

Regarding code readability, you are certainly right that ifdef are not the best. But regarding future boards addition that would need the same config, it makes things as simple as adding a single line in the peripheral configuration.

@benpicco

benpicco commented Sep 2, 2019

Copy link
Copy Markdown
Contributor

Is I2C on PB6 & PB7 a common thing on nucleo boards/stm32l MCUs? I also see PB7 & PB8 used in e.g. nucleo-l433rc and PB8 & PB9 on nucleo-l152re (and many where no i2c configuration is provided yet).

@aabadie

aabadie commented Sep 2, 2019

Copy link
Copy Markdown
Contributor Author

Is I2C on PB6 & PB7 a common thing on nucleo boards/stm32l MCUs?

I could see 2 for now + the iotlab boards. #12144 is also using the same.
PB8 & PB9 are the most common among nucleo boards.

@mrusme

mrusme commented Sep 2, 2019

Copy link
Copy Markdown

I think PB6/PB7 work due to the Arduino-compatibility of the L432KC/L412KB/L031K6/... boards. However, I found STM documentation on the I2C pins very confusing, there might be another setup that could work.

However, I'm running I2C devices on these boards (with the L412KB being implemented as we're writing here) and they work with this config. Hence I didn't feel the need to investigate further.

@benpicco benpicco left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Seeing now that we already have common/stm32/include/cfg_i2c1_pb8_pb9.h it makes sense to also include this.

I'm not particularly fond of this since it makes adding more interfaces to i2c_config harder, but as there precedent for it, I think it makes sense to add this for consistency.

@aabadie

aabadie commented Sep 3, 2019

Copy link
Copy Markdown
Contributor Author

it makes adding more interfaces to i2c_config harder

Note that this file is just a default. If someone wants to use a different configuration on his nucleo, he can still copy the peripheral configuration file and modify it in the application directory.

@benpicco benpicco merged commit c082ada into RIOT-OS:master Sep 9, 2019
@aabadie aabadie deleted the pr/boards/stm32_common_i2c branch September 9, 2019 12:46
@mrusme

mrusme commented Sep 15, 2019

Copy link
Copy Markdown

Just updated to master and found out that the I2C config I implemented was replaced by this PR. Could anyone explain to me how PB8 and PB9 is supposed to be used on the L432KC/L031K6?

Screenshot 2019-09-15 at 3 35 30 PM

To put it simple: Where would I connect SCL and SDA? The config that was included as I2C config for these boards has the following connections defined:

        .scl_pin        = GPIO_PIN(PORT_B, 8),
        .sda_pin        = GPIO_PIN(PORT_B, 9),

@mrusme

mrusme commented Sep 15, 2019

Copy link
Copy Markdown

@aabadie @benpicco is it possible that a little slip of pen happend here and you actually intended to #include "cfg_i2c1_pb6_pb7.h" for the L432KC instead of #include "cfg_i2c1_pb8_pb9.h"?

@aabadie

aabadie commented Sep 15, 2019

Copy link
Copy Markdown
Contributor Author

@mrusme, indeed there was a mistake in this PR, my bad sorry. I just opened #12235 for the fix.

@kb2ma kb2ma added this to the Release 2019.10 milestone Sep 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: boards Area: Board ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants