drivers: unify default parameter definition scheme

[haukepetersen]

Looking at the existing scheme we use when defining a device's default parameters, there seem to be a slight variation of styles used -> comment style for default values, xx_CUSTOM vs. xx_BOARD vs. xx_DEFAULT...

Furthermore, we have a source of error in all of them: in case there is more than one device defined by a board (-> xx_BOARD has more than one entry), the SAUL reg info field has still only a single one. This leads to out-of-bound access to the SAUL info array during initialization...

So I propose to unify all default parameter files to the following scheme:

/**
 * @name   Default configuration parameters for device DEVX
 * @{
 */
#ifndef DEVX_PARAM_ABC
#define DEVX_PARAM_ABC ...
#endif
#ifndef DEVX_PARAM_TTT
#define DEVX_PARAM_TTT ...
#endif

#ifndef DEVX_PARAMS
#define DEVX_PARAMS       { .abc = DEVX_PARAM_ABC, \
                            .ttt = DEVX_PARAM_TTT }
#endif

#ifndef DEVX_SAULINFO
#define DEVX_SAULINFO      { .name = "devx" }
#endif
/** @} */

/**
 * @brief   DEVX configuration
 */
static const devx_params_t devx_params[] = {
    DEVX_PARAMS
};

/**
 * @brief   SAUL registry entires
 */
static const saul_reg_info_t devx_saul_reg_info[] = {
    DEVX_SAULINFO
};

This way, if a board or application can override the device config by re-defining the DEVX_PARAMS and DEVX_SAULINFO.

We can further add a check to the auto-init code, that makes sure that the number of defined devices and the number of defined SAUL infos matches:

#define INFO_NUM    (sizeof(devx_saul_reg_info) / sizeof(devx_saul_reg_info[0]))
#define PARAM_NUM   (sizeof(devx_params) / sizeof(devx_params[0]))
assert(INFO_NUM == PARAM_NUM);

What do you guys think about this?

[smlng]

You're right regarding the inconsistent naming (and usage), which should be adapted. However, I always thought it would (and should be possible) to have multiple devices of the same kind, i.e., there is an onboard sensor and I want to attach a second sensor of the same kind. Hence, it would be

/**
 * @brief   DEVX configuration
 */
static const devx_params_t devx_params[] = {
#ifdef DEVX_PARAMS_BOARD
DEVX_PARAMS_BOARD,
#endif
#ifdef DEVX_PARAMS_CUSTOM
DEVX_PARAMS_CUSTOM
#endif
};

Though, I'm unsure if that's even possible, i.e., this would require specific address config for I2C for instance.

[haukepetersen]

Yes, it is (and should be) possible to define more than one device of a type. Using the style I proposed above, you would simply do the following in e.g. your board.h:

#define DEVX_PARAMS   { .abc = foo, .ttt = bar }, \
                      { .abc = tut, .ttt = muh }

But now that I look at this, we could maybe pull the arrays outer parenthesis into the define, to make it better readable:

// default:
#ifndef DEVX_PARAMS
#define DEVX_PARAMS      {{ .abc = DEVX_PARAM_ABC, \
                          { .ttt = DEVX_PARAM_TTT }}
#endif

// optional override by the board:
#define DEVX_PARAMS   {{ .abc = foo, .ttt = bar }, \
                       { .abc = tut, .ttt = muh }}

// leading to
static const devx_params_t devx_params[] = DEVX_PARAMS;

[smlng]

But having the complete array in DEVX_PARAMS plus having that in board.h has the drawback, that I have to change board.h instead of just providing DEVX_PARAM_CUSTOM with my application code (app/include/devx_params.h); while still having DEVX_PARAMS_BOARD.

[haukepetersen]

No, you can still override the whole file by supplying a devx_params.h file with your application (or your board). The way you can customize the configuration (as stated here) does not change.

[smlng]

I think we are not on the same page (yet)? I know that I can override the config by shipping devx_params.h and/or override some stuff by setting certain params via CFLAGS.

However, my suggestions was to specify the array as:

static const devx_params_t devx_params[] = {
#ifdef DEVX_PARAMS_BOARD
DEVX_PARAMS_BOARD,
#endif
#ifdef DEVX_PARAMS_CUSTOM
DEVX_PARAMS_CUSTOM
#endif
};

with that I can have a board specific config (for an onboard device) and/or a custom application config for an additional device of the same kind - or even none. Think of the PhyNode with the HDC1000 and now I want to attach another HDC1000 with jumper wires. The pba-d-01-kw2x provides the DEVX_PARAMS_BOARD and my app provides DEVX_PARAMS_CUSTOM.

[haukepetersen]

Ah, now I see. I think this scenario is quite rare (though not unrealistic) and for such a case I think defining a custom xx_params.h file in the application and adding the board specific configuration to it would be the way to go.

The suggestion you proposed above has IMHO some disadvantages:

• the default configuration (defined in DEVX_PARAMS_CUSTOM -> maps to DEVX_PARAMS in my suggestion) is always compiled in and not overridden by DEVX_PARAMS_BOARD
• when adding a custom devx_params.h file to an application, the default file is not used anyway

But thinking about this, we should not add the 'outer' parenthesis to the _PARAMS define as I propose last. So keeping it as proposed first, we can do

static const devx_params_t devx_params[] = {
    DEVX_PARAMS_BOARD,
    { .abc = blubb, .ttt = muh }
};

in the custom, application specific devx_params.h file.

[smlng]

we should not add the 'outer' parenthesis

    DEVX_PARAMS_BOARD,
    { .abc = blubb, .ttt = muh }

yeah, that's better!

[haukepetersen]

Ok, so this would lead to the template I first suggested on top, do you agree?

[smlng]

well, yes ... but, now I'm convinced, too 😄

[jnohlgard]

I agree that it would be good to do some clean up in the driver params code, but I dislike introducing a lot of new preprocessor defines. I have seen in our existing tree how old defines remain in the header files even though the driver using them has been refactored to use a different configuration. Putting the configuration directly in the struct definitions lead to compilation errors if the driver is refactored, and the configuration will have to be cleaned up immediately, preventing this kind of pollution. I don't have a clear solution on how to make it easy to provide application specific additions or modifications when using this method though.

[haukepetersen]

I agree with @gebart, that the macro base device configuration is not perfect. But on the other hand, this is the only working and configurable solution that we have found so far.

So I would maybe split this in to phases: 1: we unify the params files we have so far using the style proposed above by keeping the existing concept 2: we think about alternative concepts and switch over when/if we find something better

[aabadie]

@haukepetersen, @smlng, @kYc0o, are you interested in giving me some review help with all the related PRs ? Some are very simple now and should be very simple to get in. Just check that the modified macro names (XXX_PARAMS) are not use somewhere else in the code base (like in board.h).

[kYc0o]

I'm on them and though some are simple, others need more review since you introduce big (necessary) changes, which are not trivial.

It's a lot (like the first PR) but at least now it's easier to review one by one. I'll keep you updated.

Maybe we need to add some check boxes in this issue or a new one to track the progress and keep people informed about the API changes.

[aabadie]

To simplify the tracking of remaining PRs, I put here the related list:

• [ ] lsm303dlhc: #8699
• [ ] l3g4200d: #8698
• [ ] lis3dh : #8697
• [ ] isl29020: #8688
• [ ] tcs37727: #8683
• [ ] tsl2561: #8673
• [ ] lps331ap: #6577
• [ ] mag3110: #8758

[aabadie]

Maybe @kYc0o you want to have a look ?

[kYc0o]

Do you really want to get them in while the I2C embargo is ongoing?

[aabadie]

That would be nice, I won't have to rebase them after :)

[aabadie]

And the embargo is at a very early stage (waiting for CPU implementations).

[aabadie]

Another solution would be to close those PRs and to reopen them on the embargo branch

[aabadie]

After the I2C rework, the remaining adapted drivers were merged. So I consider this issue as resolved and will close it. Feel free to reopen if you disagree.