ili9341: Initial import of ili9341 LCD driver by bergzand · Pull Request #9948 · RIOT-OS/RIOT

bergzand · 2018-09-17T08:37:23Z

Contribution description

Simple driver and test application of the ili9341

Testing procedure

Test application included

Review notes

I've wrote this driver quite a while ago, it might be missing some of the newer coding practices.

Issues/PRs references

None

aabadie · 2018-09-17T08:44:40Z

Yeah! Thanks @bergzand :)
I'll test this one asap.

bergzand · 2018-09-17T14:07:14Z

@aabadie Which board had this display on it? And what is the most useful default pinout for it?

aabadie · 2018-09-17T14:12:03Z

Which board had this display on it?

stm32-f429i-disc1

And what is the most useful default pinout for it?

Look at this branch and in particular, this commit, the second commit also provides some cleanup

(edit: fixed the commit links)

aabadie

Had a quick pass of review and found minor things. I also think some parts of the code could be uncrustified (missing spaces around some operators).
Otherwise, see my previous comment about having default params for the stm32f429i-disc1 board and params initialization.
The test application also needs a README.
And finally, one question: the driver doesn't allow to display images. Do you have an idea how this could be done ?

aabadie · 2018-09-17T14:14:36Z

drivers/ili9341/Makefile

@@ -0,0 +1,2 @@
+MODULE = ili9341


This line is not needed

Will remove

still unaddressed

Whoops, fixed

aabadie · 2018-09-17T14:16:36Z

drivers/include/ili9341.h

+} ili9341_t;
+
+
+int ili9341_init(ili9341_t* dev, ili9341_params_t* prms);


Missing doxygen documentation

Hey, I checked for that, will fix 😄

bergzand · 2018-09-18T10:02:06Z

And finally, one question: the driver doesn't allow to display images. Do you have an idea how this could be done ?

The driver works by selecting an area on the display and update all pixels in that area. This closely match the lower level hardware. So yes, an image is possible, but I'd have to generate something from a bitmap.

Say we have an image of the RIOT logo of X by Y pixels, we can use the ili9341_map function with x2-x1==X and y2-y1==Y to stream the pixels to build the logo to the device. *colors just needs to be an array of pixels, X*Y long, to build the logo.

If I have some time I can try to build it, otherwise feel free to open an PR on my branch :)

aabadie · 2018-09-18T12:56:24Z

By the way, I tested this PR (with my 2 extra commits applied) and it works like a charm.

x3ro · 2018-10-05T12:26:21Z

Just verified that this also works fine on the stm32f429-disco. A few things I noticed:

For the pin defines, you use a different wording than the ST SDK uses in the case of DC (yours) vs WRX (SDK and source I found). I have no idea which one is "the better" name, just wanted to point it out. DC confused me a bit because I immediately expanded it to "direct current" :D
Apparently the reset pin is not connected on the stm32f429-disco according to this. I set it to an arbitrary pin and it seems to work. What is this used for? Do I need it?

Just for reference, I used this configuration in :

BOARD=stm32f429i-disc1
TEST_SPI ?= SPI_DEV\(0\)
TEST_SPI_CS ?= GPIO_PIN\(PORT_C,2\)
TEST_SPI_CLK ?= SPI_CLK_10MHZ
TEST_PIN_DC ?= GPIO_PIN\(PORT_D,13\)
TEST_PIN_RST ?= GPIO_PIN\(PORT_A,7\)

I took the pin assignment from here.

aabadie · 2018-10-05T12:46:53Z

@x3ro, can you try this branch ? I tested it on the disc1 version. It uses the right way of passing driver parameters and I'd like @bergzand to include my changes in his PR (I don't know why I can't open a PR on top of this one)

bergzand · 2018-10-05T12:55:02Z

@aabadie I have some time this weekend to work on the driver, just have to build a small setup to test the device.

x3ro · 2018-10-06T05:18:01Z

Hey @aabadie, yeah your branch works fine. I'm just wondering whether the pin information should really go in the driver, given that it's quite board specific. Also, why are you using magic numbers for the ports instead of e.g. PORT_A?

aabadie · 2018-10-13T18:18:46Z

Any news here @bergzand ?

I'm just wondering whether the pin information should really go in the driver, given that it's quite board specific

I would say that this is not required. Maybe a comment in the driver would be enough. It just that this screen is provided by this board, which is already available in RIOT.

why are you using magic numbers for the ports instead of e.g. PORT_A?

If we use PORT_A the driver won't build with some architectures like sam0 which uses PA. This could be fixed by the way.

x3ro · 2018-10-13T18:32:58Z

@aabadie That would also be fixed by removing the pin info from the driver and putting it in the board, right? Or in other words, not define ILI9341_PARAM_* in the driver at all.

aabadie · 2018-10-13T19:28:07Z

Or in other words, not define ILI9341_PARAM_* in the driver at all.

What do you mean exactly ? using GPIO_UNDEF ?

bergzand · 2018-10-14T10:57:19Z

Any news here @bergzand ?

Let's start with a rebase here to have all the new board names.

bergzand · 2018-10-14T17:45:48Z

* For the pin defines, you use a different wording than the ST SDK uses in the case of `DC` (yours) vs `WRX` (SDK and source I found). I have no idea which one is "the better" name, just wanted to point it out. `DC` confused me a bit because I immediately expanded it to "direct current" :D

The name of the pin is indeed WRX, but it only has the WRX function when using the parallel interface. When using the 4-line system (AKA SPI), it is called D/CX.

Apparently the reset pin is not connected on the stm32f429-disco according to this. I set it to an arbitrary pin and it seems to work. What is this used for? Do I need it?

I've made the reset pin optional, GPIO_UNDEF is now a valid value for the pin and will result in the hardware reset not being used. For my setup this doesn't work as the device requires a hardware reset before initialization, but if the reset of the board is attached to the reset of the display it might be good enough.

bergzand · 2018-10-14T17:57:33Z

@aabadie I think I've covered all your issues.

I've added an little endian mode in case the data is in little endian format. this slows down the driver a bit, so big endian is preferred, but it is useful to have when the source data/image is in little endian format.

bergzand · 2018-10-14T17:59:30Z

There is one remaining issue and that is that I can't seem to read data from the display. While not necessarily an issue when using the device, it would be nice to have.

@aabadie Could you please test if reading data from the display works for you? I'm not sure if it is because of my hardware or that I'm doing something wrong in the driver.

I'd rather not block this PR on this bug since it is not really critical for using the display.

silkeh · 2018-10-14T18:37:27Z

drivers/ili9341/ili9341.c

+    spi_release(dev->params.spi);
+}
+
+static void ili9341_set_area(ili9341_t *dev, uint16_t x1, uint16_t x2,


Maybe it is an idea to add this to the public API. Together with a write function that allows one to set n bytes in memory this would allow the writing of an area in multiple chunked writes. I have no use case for this, so feel free to ignore.

We can always extend the API in a follow up PR :)

silkeh · 2018-10-14T18:39:12Z

drivers/include/ili9341.h

+ * @param[in]   dev     device descriptor
+ * @param[in]   cmd     command
+ * @param[in]   data    data from the device
+ * @param[in]   len     length of the returned data


Seems to me that data is data to the device and len is the length of that data.

silkeh · 2018-10-14T18:40:22Z

drivers/include/ili9341.h

+ * @param[in]   y2      y coordinate of the opposite corner
+ * @param[in]   color   array of colors to fill the area with
+ */
+void ili9341_map(ili9341_t *dev, uint16_t x1, uint16_t x2, uint16_t y1,


I don't think the function name is clear. Maybe fill_pixels is clearer?

Additional observation: x2/y2 seem to be the the x/y-coordinate of the pixel in the opposite corner, not the opposite corner itself (same for all other x2/y2's).

I don't think the function name is clear. Maybe fill_pixels is clearer?

Ack

Additional observation: x2/y2 seem to be the the x/y-coordinate of the pixel in the opposite corner, not the opposite corner itself (same for all other x2/y2's).

So a documentation fix?

If explained clearly a documentation fix is fine to me, but a code change would enable users to do things like: fill_pixels(x, y, x + picture_width, y + picture_height, picture). (instead of fill_pixels(x, y, x + picture_width - 1, y + picture_height - 1, picture), which isn't as intuitive)

Renamed to pixmap and added some docs

silkeh · 2018-10-14T18:41:30Z

drivers/include/ili9341.h

+void ili9341_fill(ili9341_t *dev, uint16_t x1, uint16_t x2, uint16_t y1, uint16_t y2, uint16_t color);
+
+/**
+ * @brief   Fill a rectangular area with an array of colors


I think 'pixels' would be clearer than 'colors'. A note to remind users that the input data should exactly match the size of the area might also be useful.

Ack and ack

aabadie

Almost forgot this PR, but I still like it :)

Any chance that you could update it before the next feature freeze (18/01) ? There are unaddressed comments from @silkeh and a README is still missing for the test application.

aabadie · 2019-01-11T16:45:03Z

drivers/ili9341/ili9341.c

+
+int ili9341_init(ili9341_t *dev, const ili9341_params_t *prms)
+{
+    memcpy(&dev->params, prms, sizeof(ili9341_params_t));


Suggested change

memcpy(&dev->params, prms, sizeof(ili9341_params_t));

dev->params = *prms; /* maybe also rename prms to params ? */

aabadie

I tested this PR and it's still working well, except a minor issue with the RIOT logo (see my comment below).

@bergzand do you think you can update/rebase your PR so we can move forward with this ?

aabadie · 2019-05-08T15:57:46Z

tests/driver_ili9341/riot_logo.h

@@ -0,0 +1,71 @@
+const uint16_t picture[69][128] = {


Instead of 69, I had to use 70 with an extra empty line at the end of array to avoid having an alone green pixel at the bottom right of the logo.
I have no idea why it was there. One could think of a division by 2 issue but there is no such thing in ili9341_map implementation.

It's an off-by-one here: https://github.com/RIOT-OS/RIOT/pull/9948/files#diff-601d0bbb085a269ae7e4f7d2ac5fe68bR257-R258

And fixed, I was able to reproduce your issue and it should be fixed in the latest iteration.

aabadie · 2019-10-24T08:20:21Z

@bergzand, ping! There are still unaddressed comments and this PR would be useful for #12552.

kaspar030 · 2019-11-01T12:46:15Z

The ST7789V used on the PineTime needs spi mode 3. I git a commit adding that (https://github.com/kaspar030/RIOT/tree/ili9341_spi_mode). Maybe cherry-pick in here?

kaspar030 · 2019-11-01T12:46:48Z

Otherwise, please address @aabadie's problem and rebase and squash. ;)

bergzand · 2019-11-06T18:19:07Z

@kaspar030 @aabadie: Does it still make sense to name the driver ili9341? There are with @kaspar030's addition multiple different TFT display chips making use of this driver. Maybe some generic name is better in this case, but we can leave this rename for a follow up.

silkeh · 2019-11-06T18:39:46Z

drivers/include/ili9341.h

+ * @brief   Device descriptor for a ili9341
+ */
+typedef struct {
+    ili9341_params_t params;          /**< I2C device that sensor is connected to */


bergzand · 2019-11-06T21:19:31Z

Rebased, fixed some issues with the driver, also cleaned a few things up

bergzand · 2019-11-06T21:19:53Z

@aabadie @kaspar030: Still okay to squash?

bergzand · 2019-11-06T21:31:16Z

Maybe cherry-pick in here?

Cherry picked!

aabadie · 2019-12-07T19:04:06Z

Please squash and rebase. Just a suggestion that could be nice to use with the test application: put the RIOT logo in a blob and include it to the build ? That would make it easy for testing with other images. What do you think ?

bergzand · 2019-12-08T14:14:11Z

Just a suggestion that could be nice to use with the test application: put the RIOT logo in a blob and include it to the build ?

I think this is not that easy, so I'd rather leave this for somebody else to follow up on. To use the blob Makefile construct, the supplied file needs to be the raw image in rgb 565 format without any metadata. I don't think there is any advantage to including that instead of the image in C header format. What I'd rather see is something similar to the blob construct, but for png images, add a number of PNG images to a Make variable and the build will convert them to C header format in raw rgb 565 format. Then we could even include a repo with RIOT logo images as external package.

bergzand · 2019-12-08T14:28:00Z

@aabadie Rebased and squashed!

aabadie · 2019-12-09T07:01:47Z

@bergzand, Murdock is reporting some remaining issues. Can you have a look ?

bergzand · 2019-12-11T10:24:55Z

@aabadie I've excluded the image draw from the test when using AVR microcontrollers. On the AVR architecture, the image data ends up in the RAM. The approximately 16KB of image data doesn't fit on those devices.

Storing them in flash would require something along the lines of the PROGMEM macro which is out of scope here :)

bergzand · 2019-12-11T11:09:44Z

@aabadie I've added the stm32f030f4-demo to the insufficient memory list. This is the only board that causes build failures for now. Murdock says all green (except for the static test).

Is it okay to squash again?

tests/driver_ili9341/Makefile

aabadie · 2019-12-11T15:01:00Z

Please squash!

This commit adds support for the ili9341 display

bergzand · 2019-12-11T15:19:06Z

Squashed!

fjmolinas · 2019-12-11T15:29:52Z

Tested on stm32429i-disc, all good.

aabadie · 2019-12-11T15:33:55Z

Tested on stm32429i-disc, all good.

Thanks @fjmolinas !

aabadie

ACK!

aabadie · 2019-12-11T15:43:40Z

All green, go!

bergzand · 2019-12-11T15:50:26Z

🎉
Thanks @aabadie!

bergzand added Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: drivers Area: Device drivers labels Sep 17, 2018

bergzand requested a review from aabadie September 17, 2018 08:37

aabadie requested changes Sep 17, 2018

View reviewed changes

bergzand force-pushed the pr/ili9341 branch from 66cecd0 to f73add3 Compare October 14, 2018 10:56

silkeh reviewed Oct 14, 2018

View reviewed changes

aabadie requested changes Jan 11, 2019

View reviewed changes

silkeh mentioned this pull request Feb 28, 2019

WIP: IL3829 e-Paper/e-Ink display driver #11078

Closed

aabadie reviewed May 8, 2019

View reviewed changes

bergzand mentioned this pull request Oct 23, 2019

boards: add initial support for the PINE64 PineTime smartwatch #12552

Merged

silkeh reviewed Nov 6, 2019

View reviewed changes

bergzand force-pushed the pr/ili9341 branch from 3d775aa to d56f588 Compare November 6, 2019 21:18

bergzand force-pushed the pr/ili9341 branch 2 times, most recently from 9d0cde8 to 1c047b8 Compare December 8, 2019 14:24

aabadie added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 8, 2019

aabadie reviewed Dec 11, 2019

View reviewed changes

tests/driver_ili9341/Makefile Outdated Show resolved Hide resolved

bergzand and others added 3 commits December 11, 2019 16:18

ili9341: Initial include

ef4dcb8

This commit adds support for the ili9341 display

ili9341: Add simple test application

9c520fd

drivers/ili9341: add spi mode configuration option

35737a2

bergzand force-pushed the pr/ili9341 branch from fcb28f3 to 35737a2 Compare December 11, 2019 15:18

fjmolinas added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Dec 11, 2019

aabadie approved these changes Dec 11, 2019

View reviewed changes

aabadie merged commit a9bf691 into RIOT-OS:master Dec 11, 2019

bergzand deleted the pr/ili9341 branch December 11, 2019 15:50

fjmolinas added this to the Release 2020.01 milestone Dec 13, 2019

		} ili9341_t;


		int ili9341_init(ili9341_t* dev, ili9341_params_t* prms);

	memcpy(&dev->params, prms, sizeof(ili9341_params_t));
	dev->params = prms; / maybe also rename prms to params ? */

Conversation

bergzand commented Sep 17, 2018

Contribution description

Testing procedure

Review notes

Issues/PRs references

Uh oh!

aabadie commented Sep 17, 2018

Uh oh!

bergzand commented Sep 17, 2018

Uh oh!

aabadie commented Sep 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bergzand commented Sep 18, 2018

Uh oh!

aabadie commented Sep 18, 2018

Uh oh!

x3ro commented Oct 5, 2018

Uh oh!

aabadie commented Oct 5, 2018

Uh oh!

bergzand commented Oct 5, 2018

Uh oh!

x3ro commented Oct 6, 2018

Uh oh!

aabadie commented Oct 13, 2018

Uh oh!

x3ro commented Oct 13, 2018

Uh oh!

aabadie commented Oct 13, 2018

Uh oh!

bergzand commented Oct 14, 2018

Uh oh!

bergzand commented Oct 14, 2018

Uh oh!

bergzand commented Oct 14, 2018

Uh oh!

bergzand commented Oct 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silkeh Oct 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silkeh Oct 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silkeh Oct 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

aabadie commented Sep 17, 2018 •

edited

Loading

silkeh Oct 14, 2018 •

edited

Loading

silkeh Oct 14, 2018 •

edited

Loading

silkeh Oct 15, 2018 •

edited

Loading

aabadie commented Oct 24, 2019 •

edited

Loading