I wish we had an easy way to compare this mechanism with a corresponding function pointer based version.

I mean, if there'd be inline wrappers like:

static int disp_dev_get_max_width(dev)
{
  if !(dev->ops->get_max_width) {
    return -ENOTSUP;
  }
  return dev->ops->get_max_width(dev);
}

(this would be shared by all).

As the ops struct is constant, only supported functions would need to be mentioned in this:

const disp_dev_driver_t ili9341_disp_dev_driver = {
    .map = _ili9341_map,
    .get = _ili9341_get,
    .set = _ili9341_set,
};

I'd assume that the code would be of similar size, but always fully type checked.
(Unless we espect a lot of not implemented operations.)

Also, if a function is implemented (by implementing the corresponding static int _ili9341_scroll_down() or whatever, the compiler comlplains about an unused function if it is not added to the ops struct.

So if I understand correctly you want to test it against something like:

const disp_dev_driver_t ili9341_disp_dev_driver = {
    .map = _ili9341_map,
    .get_width = _ili9341_get_width,
    .get_heigth = _ili9341_get_heigth,
    .set_brightness = _ili9341_set_brightness,
    …
};

?

Yes, thats what I mean. I know we use this in netdev, but there it is also meant to work over netapi / msgs.

I'll try to sketch it, to get an idea of the overhead of an implemented / unimplemented function.

I would prefer to keep the ops struct as it is implemented in the PR and add wrapper functions around each of them.
I see a benefit in API usage. e.g.

disp_dev_map(dev, ...)
disp_dev_get(dev, DISP_OPT_<option name>, ...)
disp_dev_set(dev, DISP_OPT_<option name>, ...)

instead of:

dev->driver->map(dev, ...)
dev->driver->get(dev, DISP_OPT_<option name>, ...)
dev->driver->set(dev, DISP_OPT_<option name>, ...)

disp_dev_get(dev, DISP_OPT_<option name>, ...)
disp_dev_set(dev, DISP_OPT_<option name>, ...)

I think the detail is in the .... As proposed, this would be:

disp_dev_get(disp_dev_t *disp_dev, disp_opt_t opt, void *value, size_t max_len);

The user needs to manually typecheck the pointer and max_len, or check the result. Sure, the asserts catch that, but it is clumsy.

E.g., getting DISP_OPT_MAX_WIDTH:

int max_with;
int res = disp_dev_get(dev, DISP_OPT_MAX_WIDTH, &max_width, sizeof(max_width));

Do you know if this works as expected on a 32bit platform? The compiler won't check any type.

That can surely be wrapped:

static inline uint16_t disp_dev_get_max_width(dev)
{
    int16_t max_width;
    int res = disp_dev_get(dev, DISP_OPT_MAX_WIDTH, &max_width, sizeof(max_width));
   if (res) return 0;
   return max_width;
}

... but at that point my proposal is much less hassle.

Again, using pointers & max_len was a kludge in netdev, in order to allow calling the API directly over netapi / messages edit as the values need to be packed into pointers / raw data anyways.

I don't know how this would affect code size. I could imagine that having an extra function for returning a single value might be a waste. Then again, with a static inline getter, that can easily be changed to be a static field, for common values. (static inline disp_dev_get_max_width(dev) return dev->params->max_width;).

@aabadie do you see my point? I can try changing the branch towards fully function pointer based, to get an idea of the code size implications.

Do you know if this works as expected on a 32bit platform? The compiler won't check any type.

I'm not sure to understand what you mean. Is it because the example you provide uses int instead of uint16_t ?

The problem with specific wrappers is that it contraints the available common functions. And not all display device will provide get/set_brightness, get/set_color_depth, etc. The only common minimum is about width/height attributes.

If you agree and if I understand it correctly, I can try your proposed approach, and give some comparison.

I'm not sure to understand what you mean. Is it because the example you provide uses int instead of uint16_t ?

yes.

The problem with specific wrappers is that it contraints the available common functions.

This I don't understand.

And not all display device will provide get/set_brightness, get/set_color_depth, etc. The only common minimum is about width/height attributes.

The price for a not supported function is the size of one pointer, in flash. Unless we'd have a high ratio of those, it could be acceptable.

If you agree and if I understand it correctly, I can try your proposed approach, and give some comparison.

If you factor out the functions of the current switch-based approach, it could be easy to switch.

e.g.:

switch (option):
    case DISP_DEP_GET_FOO:
      ...
      ((uint16_t*)ptr) = dev->foo;
    break;

->

static uint16_t _get_foo(dev) {
  return dev->foo;
}

switch (option) {
   case DISP_DEP_GET_FOO:
      ...
      ((uint16_t*)ptr) = _get_foo(dev);
    break;
    ...

Up to here it should actually result in the same code, but now _get_foo() can directly be used in the drivers struct.

@kaspar030, I tried your proposal and it's 60B smaller than the current one. I'll push the changes in a moment.

I guess this needs a blacklist for CI tests? Or can a display tests be reasonably tested on headless nodes?

A display can be wired to any board with SPI.

So they can be compile tested, but why run the test script?

There's no test script :)

I see. So we avoid the issue (and convention: no new test application without test script).

So we avoid the issue (and convention: no new test application without test script)

It's the same for almost all driver. You can't automate the test unless the hardware is wired to the board.

Seeing this is a bit painful.

It screams for a "fill(x,x2,y,y2, color)" that doesn't do a map() for every pixel. But we can leave that to a followup.

Iterating linewise (first all x=0 y=0, 1, 2, ... then all x=1, y=0, 1, 2, ...) is also a textbook example for missing optimization. Please change to for (y, ...) for (x...).

Please change to for (y, ...) for (x...)

Done but doesn't make much difference in speed.

It screams for a "fill(x,x2,y,y2, color)"

Indeed, we also had this discussion with @bergzand IRL. We came to the conclusion that map can also be added in this situation with an input buffer. The input doesn't necessarily need to have a size equal to the size of the screen but then that would require rectangle size computation and multiple calls to map in users code (so not very handy, but more generic).

The only way to speed this up is to use a buffer with the pixels for the selected color and transmit those. Even a buffer of only 32 pixels (64 bytes) already speeds this up tremendously.

This because (at least for the ili9341), each map() first sends x1, x2, y1, y2 (8 bytes) to the display and then the pixels (another 2 byte). That's 10 bytes of SPI data for a single transfer. Transmitting a buffer of 32 pixels (64 byte + 8 byte coordinates = 72 bytes) at once is a lot more efficient in terms of SPI transfers.