Skip to content

cpp: fix board I2C configs for C++ support#9571

Merged
dylad merged 14 commits intoRIOT-OS:new_i2c_if3from
smlng:pr_i2c/periph_conf_cpp
Jul 13, 2018
Merged

cpp: fix board I2C configs for C++ support#9571
dylad merged 14 commits intoRIOT-OS:new_i2c_if3from
smlng:pr_i2c/periph_conf_cpp

Conversation

@smlng
Copy link
Copy Markdown
Member

@smlng smlng commented Jul 13, 2018

Contribution description

The I2C configuration structs of several boars causes problems when uses with C++, e.g., attributes were missing or not initialised in correct order.

Issues/PRs references

@smlng smlng 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 TF: I2C Marks issues and PRs related to the work of the I²C rework task force labels Jul 13, 2018
@smlng smlng requested a review from dylad July 13, 2018 09:25
Copy link
Copy Markdown
Member

@dylad dylad left a comment

Choose a reason for hiding this comment

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

Checked all files.
ACK and go !

@smlng
Copy link
Copy Markdown
Member Author

smlng commented Jul 13, 2018

wait for murdock!

@dylad dylad merged commit 210e27a into RIOT-OS:new_i2c_if3 Jul 13, 2018
@dylad
Copy link
Copy Markdown
Member

dylad commented Jul 13, 2018

Let's merge it !!!

.speed = I2C_SPEED_NORMAL,
.scl_pin = GPIO_PIN(PORT_B, 6),
.sda_pin = GPIO_PIN(PORT_B, 7),
.scl_af = GPIO_AF_OUT_OD,
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.

all those changes are useless since af cannot be changed on STM32 F1.
Normally, the scl_af and sda_af attributes are not used when the CPU is stm32f1.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This change is needed because of CPP check, @smlng also modified the driver accordingly.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@aabadie as @dylad wrote, I changed the driver accordingly omitting the #ifdef in there. However, I agree with you as this is kind of non-dynamic and fixed for stm32f1 CPUs it should not be exposed in the configuration of the board, which suggests it might be dynamic. But this is a general problem in many drivers and their config: i.e, that RIOT exposes params in configs that are actually not configurable at all.

Nevertheless, C++ requires to init all struct attributes properly and in order, omitting one or more or changing the order will result in compile errors. Hence I had to expose the scl/sda_af in the config.

@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented Jul 13, 2018

Hence I had to expose the scl/sda_af in the config

and this is bad. look at uart and spi structs in periph_common header, the af attributes are skipped for F1, the same should be done for I2C. I think something went wrong there during the rebase.

@dylad
Copy link
Copy Markdown
Member

dylad commented Jul 13, 2018

We don't have the choice here. If we don't add scl_af sda_af to the periph_conf, CPP apps won't compile so Murdock will never be happy.

@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented Jul 13, 2018

CPP apps won't compile so Murdock will never be happy.

I know but this can be done globally in the CPU code, not in each board peripheral config.

@smlng
Copy link
Copy Markdown
Member Author

smlng commented Jul 14, 2018

this can be done globally in the CPU code

IMHO no it can't @aabadie , at least not as long as all STM CPUs share the same i2c_config_t with scl_af and sda_af attributes, then C++ requires them to be properly initialised (and in order). The only working alternative would be to have a distinct i2c_config_t for STM32F1 CPUs w/o the _af attributes, e.g. by #ifdef -- but all those #ifdefs are also kind of bad.

@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented Jul 14, 2018

I'm pretty sure it can. The same struct is used by 2 different driver implementation. Just use #ifndef CPU_FAM_STM32F1/#endif around the af attribute. Did you try that ?

@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented Jul 14, 2018

all those #ifdefs are also kind of bad.

with unified driver implementations, I don't see a better way. For me, code duplication is even worse

@dylad
Copy link
Copy Markdown
Member

dylad commented Jul 14, 2018

Why is it better to drop af params ? IMHO its better to have the struct for every family

@dylad dylad mentioned this pull request Jul 14, 2018
@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented Jul 15, 2018

I'm not saying that we don't want the struct to configure i2c F1 boards.
I'm saying that the af attributes are useless for this family because it's something that cannot be changed/configured. Look at the code in master (for iotlab boards for example), there's nothing for af modes.
The af attributes must be skipped for F1 with an #ifndef in the i2c struct definition (in cpu/stm32_common/include). This avoids code duplication in the struct initializers and save a couple of bytes. Note that the same strategy is used for uart and spi.
As I said already, the 'same' i2c struct is alreasy used for different families (2 types of drivers) and some don't use all attributes already (bus clock, etc).
@vincent-d, if you are around can you have a look and give your opinion ?

@vincent-d
Copy link
Copy Markdown
Member

I agree with @aabadie here, we should do as for uart and spi, see:

#ifndef CPU_FAM_STM32F1
with uart driver for instance. Same struct but some fields are out for F1.

@vincent-d
Copy link
Copy Markdown
Member

Forget it, I missed #9574 ;)

@cladmi cladmi added this to the Release 2018.10 milestone Aug 11, 2018
@smlng smlng deleted the pr_i2c/periph_conf_cpp branch June 25, 2019 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR TF: I2C Marks issues and PRs related to the work of the I²C rework task force 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.

5 participants