drivers: Add text display interface #52032

thedjnK · 2022-11-07T19:36:36Z

Adds the base driver include file and noritake itron text display dts file for review.

Tested:

amc2004a (20x4 HD44780-clone): ☑ Works on nucleo_f746zg in both 4 and 8-bit modes
Noritake Itron gu280x16g (Noritake itron UART): ☑ Works on nucleo_f401re
seed grove lcd rgb (Jinghua Display JHD1313): ☒ Do not have shield to test with
Noritake Itron gu24025ecpb (24x2 HD44780-clone): ☑ Works on stm32f4_disco with 800ms boot delay, sometimes displays '6', '66' or '666' at start when board reset button is pressed but this seems to be a quirk of the display/reset or other time value that needs increasing
Unbranded 1602a (16x2 HD44780-clone): ☑ Works on stm32f4_disco in 4-bit mode (not checking on 8-bit mode as the debugger deciding if it detects a core or not completely at random is almost making me throw it against a wall)

Fixes #51912

gmarull

some initial comments

include/zephyr/drivers/text_display.h

drivers/text_display/text_display_noritake_itron.c

cfriedt

I love that you're adding text-based LCDs here!

It's been a while but would be a great addition on my next demo.

Just a couple of questions / comments:

Are either of grove or itron displays capable of producing interrupts (e.g. via GPIO)? If so, I would suggest building those capabilities into the drivers.
Some of the drivers sleep within the API calls. In general, it would be better to use a semaphore or spinlock and either delayed work or an ISR to unlock

Thanks though - I'm really looking forward to seeing this upstream 😁

cfriedt · 2022-11-22T16:25:06Z

It might also be useful to wrap API calls in a __syscall wrapper (see <zephyr/drivers/gpio.h>. For kernel-mode only builds, there is very little overhead (other than some argument checking), but for user mode, it protects access to the driver data

thedjnK · 2022-11-27T18:43:36Z

Are either of grove or itron displays capable of producing interrupts (e.g. via GPIO)? If so, I would suggest building those capabilities into the drivers.

@cfriedt Not sure what you mean here by interrupt, unless you mean if they have user input/buttons/touchscreen? The noritake itron has a busy output line which I did expose here but will hide internally in the driver and not expose externally, HD44780 does not have inputs.

cfriedt · 2022-11-27T18:53:06Z

@cfriedt Not sure what you mean here by interrupt, unless you mean if they have user input/buttons/touchscreen? The noritake itron has a busy output line which I did expose here but will hide internally in the driver and not expose externally, HD44780 does not have inputs.

@thedjnK - a busy line should do for a gpio interrupt.

I could make some additional code comments if you need clarification, but my general suggestion was to avoid sleeping (ans also busy looping). Sleeping is usually bad for drivers.

Using an interrupt (e.g. a busy line) is one way to avoid sleeping, and another way to avoid sleeping is to use a workqueue. It might be smart to use a dedicated workqueue in this case.

thedjnK · 2022-11-27T18:56:10Z

@cfriedt 😆 this was the comment from the architecture meeting:

text_display_is_busy probably debatable

I just removed it so I can add it back quickly...

cfriedt · 2022-11-27T18:57:23Z

drivers/text_display/text_display_grove_lcd.c

+
+	rc = i2c_write_dt(&config->bus, clear, sizeof(clear));
+	LOG_DBG("clear, delay 20 ms");
+	k_sleep(K_MSEC(20));


Would avoid sleeping here

This is from the original zephyr driver (copy and paste) and would think sleeping is the only course since the command should be blocking until it is complete (someone raised this in the architecture meeting)

Commands can still be blocking if you use e.g. a semaphore (semaphore can be unlocked via ISR or workqueue).

This is done in a few places in e.g. sensor drivers.

Just a thought.

cfriedt · 2022-11-27T18:57:39Z

drivers/text_display/text_display_grove_lcd.c

+	rc = i2c_write_dt(&config->bus, buf, sizeof(buf));
+
+	LOG_DBG("set display_state options, delay 5 ms");
+	k_sleep(K_MSEC(5));


Would avoid sleeping here

cfriedt · 2022-11-27T18:58:19Z

drivers/text_display/text_display_grove_lcd.c

+	i2c_write_dt(&config->bus, buf, sizeof(buf));
+
+	LOG_DBG("set function options, delay 5 ms");
+	k_sleep(K_MSEC(5));


Would avoid sleeping here

cfriedt · 2022-11-27T19:01:37Z

drivers/auxdisplay/auxdisplay_grove_lcd.c

+	 * VDD to power on, so pause a little here, 30 ms min, so we go 50
+	 */
+	LOG_DBG("delay 50 ms while the VDD powers on");
+	k_sleep(K_MSEC(50));


Probably better to move this to something like an enable() function.

thedjnK · 2023-01-04T23:07:31Z

OK I've made some fixes and updates, PM is implemented in my display, added a simple sample application, still some work to do on it.

@cfriedt I'm a bit stuck with using interrupts and semaphores, I was about to switch to them on my display driver, then I stopped and paused because that would pose a problem, a problem for me at least, I want to be able to use the display from a bootloader (i.e. mcuboot) and imposing semaphores then means I would need to build mcuboot with mutlthreading support, just enabling that option without using the functions in code adds 4KB to a bootloader build which isn't a small number, and thinking about it, the delays should not really be long for a display (for mine at 38400 baud, assuming you output 40 characters and every 3 characters you have to pause for 4ms, the total time needed to display all that is about 63ms, and whilst the display is busy, other tasks can run perhaps, so at least for my driver (note: noritake display, not the groove) I don't think it's worth changing to interrupts. I don't think it's worth doing either for the groove but that's because that driver is basically a straight up copy and paste from existing zephyr code, I don't have that display to test with so don't want to submit a driver that might be broken and I'm unable to test with. For a HD44780 driver (which will have to come at a later date) I will think about it, although I think that would fall under the same bucket of using k_sleep() because I don't think those display drivers have an interrupt line either, it's all "hold these lines for a minimum time period".

cfriedt · 2023-01-04T23:16:46Z

OK I've made some fixes and updates, PM is implemented in my display, added a simple sample application, still some work to do on it.

@cfriedt I'm a bit stuck with using interrupts and semaphores.

Device Tree can fix that 😄 Make it a DT property and hook up things in your INIT.

nordicjm · 2023-01-05T12:30:35Z

@cfriedt not for setting up the GPIO, that's already done in this PR, it's for usage of semaphores, the burden of enabling that is too great for a simple bootloader

cfriedt · 2023-01-05T12:57:51Z

@cfriedt not for setting up the GPIO, that's already done in this PR, it's for usage of semaphores, the burden of enabling that is too great for a simple bootloader

@nordicjm - you can use DT properties to compile-in or compile-out support for semaphores. In the non-semaphore version just use a different function pointer for achieving the same thing. Then you get the best of both worlds :-)

gmarull · 2023-01-10T08:52:02Z

@cfriedt not for setting up the GPIO, that's already done in this PR, it's for usage of semaphores, the burden of enabling that is too great for a simple bootloader

@nordicjm - you can use DT properties to compile-in or compile-out support for semaphores. In the non-semaphore version just use a different function pointer for achieving the same thing. Then you get the best of both worlds :-)

Just saw this comment: DT describes hardware, not software configuration ;-)

cfriedt · 2023-01-10T14:16:10Z

@cfriedt not for setting up the GPIO, that's already done in this PR, it's for usage of semaphores, the burden of enabling that is too great for a simple bootloader

@nordicjm - you can use DT properties to compile-in or compile-out support for semaphores. In the non-semaphore version just use a different function pointer for achieving the same thing. Then you get the best of both worlds :-)

Just saw this comment: DT describes hardware, not software configuration ;-)

@gmarull - that's right. In this case the DT would be describing whether or not there are interrupts available for the device (so business as usual) and then the driver would be configured to either use an interrupt-driven routine or polling based on that (also pretty normal, right?).

I'm just pointing out that it can be done in a compile-time const way inside of DT_INST_FOREACH_STATUS_OKAY(). It can be done at runtime in init() too of course.

nordicjm · 2023-01-10T14:31:53Z

@gmarull - that's right. In this case the DT would be describing whether or not there are interrupts available for the device (so business as usual) and then the driver would be configured to either use an interrupt-driven routine or polling based on that (also pretty normal, right?).

That doesn't make sense, if a display needs an interrupt then it needs an interrupt and the driver would have it, if it doesn't need an interrupt then it wouldn't be present in the driver. If there is an optional one then it would down to the driver if it used it or not (and with displays, they are not optional, the display I have here will happily print complete garbage if you do not use and respect the busy line). And from a dts perspective, it's just a phandle-array

None of which has any relation to multithreading.

Addressed

Ports the Jinghua Display JHD1313 LCD (with RGB backlight) driver to use the new auxdisplay driver interface. This driver is used on the seeed grove LCD RGB display, and replaces it. Signed-off-by: Jamie McCrae <spam@helper3000.net>

gmarull · 2023-05-26T19:47:52Z

drivers/auxdisplay/auxdisplay_hd44780.c

+		.capabilities = {								\
+			.columns = DT_INST_PROP(inst, columns),					\
+			.rows = DT_INST_PROP(inst, rows),					\
+			.mode = DT_ENUM_IDX(DT_DRV_INST(inst), mode),				\


gmarull · 2023-05-26T19:48:28Z

drivers/auxdisplay/auxdisplay_hd44780.c

+		.clear_delay = DT_INST_PROP(inst, clear_command_delay_us),			\
+		.boot_delay = DT_INST_PROP(inst, boot_delay_ms),				\
+	};											\
+	PM_DEVICE_DT_INST_DEFINE(inst, auxdisplay_hd44780_pm_action);				\


gmarull · 2023-05-26T19:51:57Z

samples/drivers/auxdisplay/README.rst

+
+This sample shows how to use the auxiliary display drivers by outputting a
+sample "Hello World" text to one. If power management is enabled when
+building (:kconfig:option:`CONFIG_PM_DEVICE`), it also demonstrates


gmarull · 2023-05-26T19:52:22Z

samples/drivers/auxdisplay/src/main.c

+
+	k_sleep(K_SECONDS(3));
+
+	pm_device_action_run(dev, PM_DEVICE_ACTION_SUSPEND);


needs update?

Adds an auxiliary display driver for Hitachi HD44780-based (and compatible) LCD displays. Signed-off-by: Jamie McCrae <spam@helper3000.net>

Add myself as auxiliary display maintainer. Signed-off-by: Jamie McCrae <spam@helper3000.net>

Adds a simple auxdisplay sample which outputs hello world and the name of the board. A sample overlay is provided for the nucleo_f746zg board with a Hitachi HD44780-compatible display. Signed-off-by: Jamie McCrae <spam@helper3000.net>

Adds base auxdisplay documentation. Signed-off-by: Jamie McCrae <spam@helper3000.net>

Adds the new auxiliary display peripheral as an unstable API. Signed-off-by: Jamie McCrae <spam@helper3000.net>

Adds a note on unstable auxiliary display support being added, alongside 3 drivers. Signed-off-by: Jamie McCrae <spam@helper3000.net>

thedjnK force-pushed the text_display_test branch from 25f01df to a157d2f Compare November 15, 2022 14:36

henrikbrixandersen self-requested a review November 22, 2022 16:09

gmarull requested changes Nov 22, 2022

View reviewed changes

cfriedt requested changes Nov 22, 2022

View reviewed changes

thedjnK force-pushed the text_display_test branch from a157d2f to c907d4c Compare November 27, 2022 18:59

cfriedt reviewed Nov 27, 2022

View reviewed changes

thedjnK force-pushed the text_display_test branch 4 times, most recently from 3104e47 to b41753d Compare November 29, 2022 22:10

thedjnK force-pushed the text_display_test branch from b41753d to ede3b70 Compare December 28, 2022 12:09

thedjnK force-pushed the text_display_test branch 3 times, most recently from e085483 to 9f54533 Compare January 4, 2023 22:55

thedjnK force-pushed the text_display_test branch from 9f54533 to b16e223 Compare January 7, 2023 22:11

cfriedt self-requested a review January 10, 2023 23:21

thedjnK force-pushed the text_display_test branch from b16e223 to 79ed8d2 Compare January 15, 2023 12:01

thedjnK removed request for cfriedt and henrikbrixandersen January 15, 2023 12:02

thedjnK dismissed cfriedt’s stale review via 86e082e May 26, 2023 17:41

thedjnK force-pushed the text_display_test branch from 184e8d9 to 86e082e Compare May 26, 2023 17:41

thedjnK requested review from gmarull, kartben and cfriedt May 26, 2023 17:42

thedjnK force-pushed the text_display_test branch 3 times, most recently from 57c1413 to a9a1ea0 Compare May 26, 2023 19:39

gmarull reviewed May 26, 2023

View reviewed changes

thedjnK force-pushed the text_display_test branch 3 times, most recently from fe815fb to ed9bc44 Compare May 26, 2023 20:14

cfriedt previously approved these changes May 26, 2023

View reviewed changes

thedjnK requested a review from gmarull May 26, 2023 20:25

thedjnK added 7 commits May 26, 2023 21:27

drivers: auxdisplay: Add Hitachi HD44780 driver

38caa64

Adds an auxiliary display driver for Hitachi HD44780-based (and compatible) LCD displays. Signed-off-by: Jamie McCrae <spam@helper3000.net>

CODEOWNERS: Add self as aux display maintainer

ec71330

Add myself as auxiliary display maintainer. Signed-off-by: Jamie McCrae <spam@helper3000.net>

MAINTAINERS: Add self as aux display maintainer

5da99e8

Add myself as auxiliary display maintainer. Signed-off-by: Jamie McCrae <spam@helper3000.net>

samples: driver: Add auxdisplay sample

6347029

Adds a simple auxdisplay sample which outputs hello world and the name of the board. A sample overlay is provided for the nucleo_f746zg board with a Hitachi HD44780-compatible display. Signed-off-by: Jamie McCrae <spam@helper3000.net>

doc: hardware: peripheral: Add auxdisplay documentation

b6f8cc1

Adds base auxdisplay documentation. Signed-off-by: Jamie McCrae <spam@helper3000.net>

doc: api: Add auxdisplay as unstable API

85cae82

Adds the new auxiliary display peripheral as an unstable API. Signed-off-by: Jamie McCrae <spam@helper3000.net>

doc: release: 3.4: Add auxdisplay addition node

6ff540c

Adds a note on unstable auxiliary display support being added, alongside 3 drivers. Signed-off-by: Jamie McCrae <spam@helper3000.net>

thedjnK dismissed cfriedt’s stale review via 6ff540c May 26, 2023 20:27

thedjnK force-pushed the text_display_test branch from ed9bc44 to 6ff540c Compare May 26, 2023 20:27

gmarull approved these changes May 26, 2023

View reviewed changes

cfriedt approved these changes May 26, 2023

View reviewed changes

kartben approved these changes May 26, 2023

View reviewed changes

carlescufi merged commit d8d119f into zephyrproject-rtos:main May 26, 2023
24 checks passed

thedjnK deleted the text_display_test branch December 10, 2023 13:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: Add text display interface #52032

drivers: Add text display interface #52032

thedjnK commented Nov 7, 2022 •

edited

gmarull left a comment

cfriedt left a comment •

edited

cfriedt commented Nov 22, 2022

thedjnK commented Nov 27, 2022

cfriedt commented Nov 27, 2022 •

edited

thedjnK commented Nov 27, 2022

cfriedt Nov 27, 2022

thedjnK Nov 27, 2022

cfriedt Nov 27, 2022

cfriedt Nov 27, 2022

cfriedt Nov 27, 2022

cfriedt Nov 27, 2022

thedjnK commented Jan 4, 2023

cfriedt commented Jan 4, 2023

nordicjm commented Jan 5, 2023

cfriedt commented Jan 5, 2023

gmarull commented Jan 10, 2023

cfriedt commented Jan 10, 2023

nordicjm commented Jan 10, 2023 •

edited

gmarull May 26, 2023

gmarull May 26, 2023

thedjnK May 26, 2023

gmarull May 26, 2023

thedjnK May 26, 2023

gmarull May 26, 2023

thedjnK May 26, 2023


		k_sleep(K_SECONDS(3));

		pm_device_action_run(dev, PM_DEVICE_ACTION_SUSPEND);

drivers: Add text display interface #52032

drivers: Add text display interface #52032

Conversation

thedjnK commented Nov 7, 2022 • edited

gmarull left a comment

Choose a reason for hiding this comment

cfriedt left a comment • edited

Choose a reason for hiding this comment

cfriedt commented Nov 22, 2022

thedjnK commented Nov 27, 2022

cfriedt commented Nov 27, 2022 • edited

thedjnK commented Nov 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thedjnK commented Jan 4, 2023

cfriedt commented Jan 4, 2023

nordicjm commented Jan 5, 2023

cfriedt commented Jan 5, 2023

gmarull commented Jan 10, 2023

cfriedt commented Jan 10, 2023

nordicjm commented Jan 10, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thedjnK commented Nov 7, 2022 •

edited

cfriedt left a comment •

edited

cfriedt commented Nov 27, 2022 •

edited

nordicjm commented Jan 10, 2023 •

edited