Skip to content
This repository has been archived by the owner on Jan 7, 2019. It is now read-only.

Use named struct instead of anonymous parameter list for drivers. #86

Closed
wants to merge 1 commit into from
Closed

Use named struct instead of anonymous parameter list for drivers. #86

wants to merge 1 commit into from

Conversation

salkinium
Copy link
Member

Via @strongly-typed:

Until now, not-so-long and long anonymous parameter lists were used to connect hardware drivers with the real hardware.

This is C-style as the order counts. If you swap Ce and Dc in the example above the code will compile and fail and it is hard to spot the error.

With a named struct as a single parameter each parameter has a distinguished name and is less error-prone. And the code is the documentation. Adding another parameter is simple, too.

The only downside is that default arguments cannot be defined.

…rameter list for template class.

Until now, not-so-long and long anonymous parameter lists were used to connect hardware drivers with the real hardware.

This is C-style as the order counts. If you swap Ce and Dc in the example above the code will compile and fail and it is hard to spot the error.

With a named struct as a single parameter each parameter has a distinguished name and is less error-prone. And the code is the documentation. Adding another parameter is simple, too.

The only downside is that default arguments cannot be defined.
@strongly-typed
Copy link
Member

Yes, I still like my idea :-)

I would suggest to use this pattern for all new drivers and classes which use more than one parameter. It does not break compatibility.

@salkinium
Copy link
Member Author

The main issue I see with the approach is documentation and the naming of the types.

With the template parameters the name is contained within the driver, it essentially doesn't matter what you call it.
With this approach you have to correctly alias the types to the ones that the driver requires. If another driver calls the thing differently, you have to rename it in your code as well.
It's like having to modify your code when I rename the name of a function argument.
I find that a very awkward process.
It's also very hard to natively document with Doxygen and C++. You'd need the concept of Traits, which isn't available in C++ (yet).

However, I would like to explore this concept for abstracting the driver logic from the bus as much as possible.
We've had issues with this before, and it resulted in the classes in driver/bus and the Lis3Transport classes.
In the case of the Nokia5110 this might be a little overkill (and we already have a "bus" abstraction from BufferedGraphicsDisplay anyways), but perhaps there are more interesting things to discover.

@salkinium
Copy link
Member Author

I've been thinking more about the applications of this. I've come to really like this idea, it can be used for a lot of things.

I think the documentation and defaults problem can be solved by making the sensor provide a (documented) base type from which the user has to inherit.
This allows proper documentation and defaulting:

struct driver::Configuration 
{
    using SpiMaster = void; // required
    using Cs = void;    // required
    using Interrupt = xpcc::GpioUnunsed;    // optional input
    static constexpr uint8_t Revision = 2;  // driver r1 has one register less than r2
    static uint16_t crc(uint16_t input) {…}   // overwrite stateless functions like CRC
};

You would then inherit your custom settings and only overwrite what you care about:

struct DriverConfiguration : public driver::Configuration 
{
    using SpiMaster = SpiMaster3;
    using Cs = GpioOutputB4;
    // not using interrupt pin
    static constexpr uint8_t Revision = 1;  // using old revision
    // default CRC is fine
};

Is this a good idea? It won't be possible to inline declare the config anymore, so it can lead to more code.
But on the other hand, you won't have more than a handful of drivers in you application anyway, so I guess this is ok.

@salkinium
Copy link
Member Author

Partially implemented, but requires further documented policy and examples to be made best practice.

@salkinium salkinium closed this Jan 7, 2018
@salkinium salkinium mentioned this pull request Mar 30, 2020
28 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
xpcc
On Hold
Development

Successfully merging this pull request may close these issues.

None yet

2 participants