[riot-notifications] [RIOT-OS/RIOT] black/white e-Paper/e-Ink display driver (#12509)

Alexandre Abadie notifications at github.com
Wed May 19 09:25:54 CEST 2021


@aabadie requested changes on this pull request.

I made another pass on this PR and I think there are genericity issues with this PR. It would be very nice if we could merge this but I would definitely remove the setup specific from the application directory and better handle the partial/full refresh logic.
I also think that the disp_dev related test application could be removed: RIOT is already supporting a board with an on-board e-ink paper display and it could be used directly with `tests/disp_dev` (with some minor Makefile changes to this application).

> @@ -247,6 +247,11 @@ ifneq (,$(filter encx24j600,$(USEMODULE)))
   USEMODULE += xtimer
 endif
 
+ifneq (,$(filter epd_bw_spi,$(USEMODULE)))

The dependency of the module driver should go in its own Makefile.dep. Same for Makefile.include.

> @@ -0,0 +1,7 @@
+SRC=epd_bw_spi.c

```suggestion
SRC = epd_bw_spi.c
```

> @@ -0,0 +1,128 @@
+/*
+ * Copyright (C) 2020 Inria

Copyright should be adapded

> +/*
+ * Copyright (C) 2020 Inria
+ *
+ * This file is subject to the terms and conditions of the GNU Lesser
+ * General Public License v2.1. See the file LICENSE in the top level
+ * directory for more details.
+ */
+
+/**
+ * @ingroup     drivers_epd_bw_spi
+ * @{
+ *
+ * @file
+ * @brief       Driver adaption to disp_dev generic interface
+ *
+ * @author      Alexandre Abadie <alexandre.abadie at inria.fr>

And also authorship

> +    assert(dev);
+
+    return dev->size_x;
+}
+
+static uint8_t _epd_bw_spi_color_depth(disp_dev_t *disp_dev)
+{
+    (void)disp_dev;
+    return EPD_BW_SPI_DISP_COLOR_DEPTH;
+}
+
+static void _epd_bw_spi_set_invert(disp_dev_t *disp_dev, bool invert)
+{
+    (void)disp_dev;
+    (void)invert;
+    assert(false);

This will break the `tests/disp_dev` application. Better do nothing (and write a comment here).

> +#ifndef EPD_BW_SPI_INTERNAL_H
+#define EPD_BW_SPI_INTERNAL_H
+
+#include "xtimer.h"
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/**
+ * @name    EPD_BW_SPI SPI commands
+ * @brief   Commands used for controlling EPD displays.
+ *          These are surprisingly portable.
+ * @{
+ */
+#define EPD_BW_SPI_CMD_DRIVER_OUTPUT_CONTROL (0x01)

Do you mind aligning all defined values vertically ? (I know I'm asking for something boring)

> +void epd_bw_spi_set_partial(disp_dev_t *disp_dev, bool partial)
+{
+    ((epd_bw_spi_t *)disp_dev)->refresh_partial = partial;
+}

I don't think you need this function. This breaks the generic nature of disp_dev (if one uses disp_dev, one won't expect to have to call driver specific functions). I would simply remove the `refresh_partial` attribute and force the partial refresh when disp_dev if used. The partial refresh could eventually be disabled at compile time with a define if disp_dev is used.

> +    #ifdef MODULE_DISP_DEV
+    disp_dev_t *dev;                    /**< Pointer to the generic display device */
+    bool refresh_partial;               /**< Use partial instead of full refreshes */
+    #endif

```suggestion
#ifdef MODULE_DISP_DEV
    disp_dev_t *dev;                    /**< Pointer to the generic display device */
#endif
```

See my comment above about the partial_refresh attribute.

> +
+/**
+* @brief   Configure the display to update with a full or partial refresh
+*
+* @param[in] dev     Device descriptor.
+* @param[in] partial Boolean to set partial or full mode.
+*/
+void epd_bw_spi_set_partial(disp_dev_t *dev, bool partial);

```suggestion
```

Same comment about `partial_refresh`.

> @@ -0,0 +1,10 @@
+BOARD ?= nucleo-f411re
+
+include ../Makefile.tests_common
+
+USEMODULE += xtimer
+USEMODULE += epd_bw_spi
+
+INCLUDES += -I$(APPDIR)

This is a configuration for your personal setup. I don't think it should be put there (I mean in RIOT). Maybe someone with a nucleo-f411re won't wire the same e-ink paper display than yours and maybe not on the same pins.
So I would simply remove the `epd_bw_spi_params.h` include from the application directory (it should remain generic as much as possible). Note that this would simply forbid the possibility to build this application with a board providing a e-ink paper: the board support won't be able to provide custom defines (different pins, screen size, controller) the usual way.
The x/y display sizes and controller are the same as the default ones, so no need to overwrite them here.

Also note that I would be ok to move the setup specific pin definitions to the default driver params with a comment (use the same one used in the application params.h file, you can even generalize to any Nucleo 64 board I guess, they have a compatible pinout).

> @@ -0,0 +1,9 @@
+include ../Makefile.tests_common
+
+USEMODULE += xtimer
+USEMODULE += epd_bw_spi
+USEMODULE += disp_dev
+
+INCLUDES += -I$(APPDIR)

Same comment as above.

I also think that this application won't be technically required once the support for the stm32l0583-discovery on-board display will be added (I have a branch based on this PR for this): the default `tests/disp_dev` application will cover this use case.

> +        /* Use full refreshes for the full-size logo */
+        epd_bw_spi_set_partial(dev, false);

Exactly what I said above: do you expect people to call driver specific functions in an application that is using a generic API ?
But now that I see how you would like to use it, better check for the bounds used with the disp_dev `map` function and call the `epd_bw_spi_update_part`/`epd_bw_spi_update_full` accordingly.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/12509#pullrequestreview-662816897
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210519/1e699967/attachment.htm>


More information about the notifications mailing list