Skip to content

drivers/*: unify drivers params definition scheme#7937

Closed
aabadie wants to merge 9 commits intoRIOT-OS:masterfrom
aabadie:pr/update_drivers_params
Closed

drivers/*: unify drivers params definition scheme#7937
aabadie wants to merge 9 commits intoRIOT-OS:masterfrom
aabadie:pr/update_drivers_params

Conversation

@aabadie
Copy link
Copy Markdown
Contributor

@aabadie aabadie commented Nov 4, 2017

This PR is an attempt to address what was suggested in #7519 and especially the first step of this comment.

I started to update some of the drivers also available in auto_init/saul but before going further, I'd like to know if I'm on the right way.

@aabadie aabadie added Area: drivers Area: Device drivers Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels Nov 4, 2017
@aabadie aabadie requested a review from haukepetersen November 4, 2017 18:59
@aabadie aabadie changed the title drivers/*: update drivers params [WIP] drivers/*: update drivers params definition scheme [WIP] Nov 4, 2017
@aabadie aabadie force-pushed the pr/update_drivers_params branch from db0b16a to 1f10b80 Compare November 5, 2017 20:25
@aabadie aabadie changed the title drivers/*: update drivers params definition scheme [WIP] drivers/*: update drivers params definition scheme Nov 5, 2017
@aabadie aabadie removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Nov 5, 2017
@aabadie
Copy link
Copy Markdown
Contributor Author

aabadie commented Nov 5, 2017

Finally, I updated all drivers and their saul adaption when available. In the mean time, I also removed some *_params.h files for some boards because I think they are useless since some values are already defined in the board.h and then are taken in to account by *_params.h in the drivers directory.

@aabadie aabadie added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 5, 2017
@aabadie aabadie force-pushed the pr/update_drivers_params branch 3 times, most recently from 150a91a to f1120b8 Compare November 5, 2017 21:41
@haukepetersen
Copy link
Copy Markdown
Contributor

Nice effort! I think most of the changes are very valid and its nice to see the style of parameter definition to be cleaned up.

One think I don't like, that this PR is still using (slightly) different styles in the same context. For example looking at the fox board: the parameters for the radio are defined using some kind of static struct initializer style

#define AT86RF2XX_PARAMS    { .spi = SPI_DEV(0),                \
                              ...
                              .reset_pin = GPIO_PIN(PORT_C, 1)}

while the on-board sensors are configured using singular defines

 #define L3G4200D_I2C        I2C_0
 #define L3G4200D_ADDR       0x68
 #define L3G4200D_DRDY       GPIO_PIN(PORT_B,8)
 #define L3G4200D_INT        GPIO_PIN(PORT_B,11)

So I would prefer to keep to one style. IMHO the singular defines are the nicest, as this allows us to use default values which we don't need to re-define. So taking the l3g4200d as example, The fox configuration for this sensor would look like:

/**
 * @name    Define the interface for the L3G4200D gyroscope
 * @{
 */
#define L3G4200D_PARAM_DRDY       GPIO_PIN(PORT_B,8)
#define L3G4200D_PARAM_INT        GPIO_PIN(PORT_B,11)
/** @} */

The I2C dev and addr values then simply fallback to their default values I2C_DEV(0) and 0x68. NOTE: the l3g4200_params.h file needs also to be adapted as it does not define default values currently...

The same would be true for the at86rf radio, whose configuration would look like this:

/**
 * @name    Define the interface to the AT86RF231 radio
 */
#define AT86RF2XX_PARAM_CS          (GPIO_PIN(PORT_A, 1))
#define AT86RF2XX_PARAM_INT         (GPIO_PIN(PORT_C, 2))
#define AT86RF2XX_PARAM_SLEEP       (GPIO_PIN(PORT_A, 0))
#define AT86RF2XX_PARAM_RESET       (GPIO_PIN(PORT_C, 1))

Here again: SPI dev and clock speed use the default values...

@aabadie
Copy link
Copy Markdown
Contributor Author

aabadie commented Nov 6, 2017

Thanks for having a look @haukepetersen.

the singular defines are the nicest, as this allows us to use default values which we don't need to re-define

I totally agree with this and will apply the required change.

@aabadie
Copy link
Copy Markdown
Contributor Author

aabadie commented Nov 6, 2017

After looking at the board params (iotlab-m3, fox, etc), the actual status of this PR is leaving them broken. I'm working on fixing them...

@aabadie
Copy link
Copy Markdown
Contributor Author

aabadie commented Nov 7, 2017

@haukepetersen, I'm nearly done with this one (at least the board.h are ready). Can you have a look again ?

@aabadie aabadie force-pushed the pr/update_drivers_params branch 3 times, most recently from 464623b to 087cc44 Compare November 7, 2017 11:36
@aabadie
Copy link
Copy Markdown
Contributor Author

aabadie commented Nov 7, 2017

Ok, after some fight, CI is finally green here... but the diff has increased significantly.

@gebart @haukepetersen, can you check that all mulle auto configured devices are still working ?
Is there someone with pba-d-01-kw2x and fox boards that can try ?

It's ok on iotlab-m3.

@aabadie aabadie force-pushed the pr/update_drivers_params branch 4 times, most recently from 1b8b5b2 to 0b8a8ef Compare November 7, 2017 12:54
@aabadie aabadie requested a review from smlng November 7, 2017 13:13
@aabadie
Copy link
Copy Markdown
Contributor Author

aabadie commented Nov 7, 2017

Maybe @smlng is also interested by this one ?

Copy link
Copy Markdown
Member

@smlng smlng left a comment

Choose a reason for hiding this comment

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

tested with PhyNode (pba-d-01-kw2x): works.

Style-wise I don't like the superfluous use of parentheses around plain macros, like in #define AT86RF2XX_PARAM_CS (GPIO_PIN(PORT_A, 1)) or #define BTN0_PORT (PORTD).

They weren't there before and are not needed. I agree to have them to guard/enclose more complex expressions, but in general for instance the macro PORTD when resolved is already enclosed in parentheses. They don't hurt, but (IMHO) they do not enhance readability in any way either.

Nevertheless this would be non-blocking from my side.

@aabadie
Copy link
Copy Markdown
Contributor Author

aabadie commented Mar 1, 2018

Closing in favor of all mentioned PRs above and the remaining ones that will come soon

I think I'm done but I'm not 100% sure. I admit I lost myself in all those branches and PRs...

Each one should be simpler to review @kYc0o ;)

But now I'm wondering what was the simplest. It will be a lot of work reviewing all that, right ?

@aabadie aabadie deleted the pr/update_drivers_params branch March 21, 2018 06:51
TorbenPetersen added a commit to ibr-cm/RIOT that referenced this pull request May 30, 2018
TorbenPetersen added a commit to ibr-cm/RIOT that referenced this pull request May 30, 2018
TorbenPetersen added a commit to ibr-cm/RIOT that referenced this pull request Jun 6, 2018
TorbenPetersen added a commit to ibr-cm/RIOT that referenced this pull request Jul 9, 2018
TorbenPetersen added a commit to ibr-cm/RIOT that referenced this pull request Aug 16, 2018
TorbenPetersen added a commit to ibr-cm/RIOT that referenced this pull request Nov 8, 2018
TorbenPetersen added a commit to ibr-cm/RIOT that referenced this pull request Mar 7, 2019
TorbenPetersen added a commit to ibr-cm/RIOT that referenced this pull request Mar 11, 2019
TorbenPetersen added a commit to ibr-cm/RIOT that referenced this pull request Mar 11, 2019
TorbenPetersen added a commit to ibr-cm/RIOT that referenced this pull request Mar 11, 2019
TorbenPetersen added a commit to ibr-cm/RIOT that referenced this pull request Oct 8, 2019
TorbenPetersen added a commit to ibr-cm/RIOT that referenced this pull request Oct 8, 2019
TorbenPetersen added a commit to ibr-cm/RIOT that referenced this pull request Nov 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: drivers Area: Device drivers Area: SAUL Area: Sensor/Actuator Uber Layer CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants