add GDEW0154x e-paper driver #902

xas · 2023-10-23T16:14:00Z

Description

add a driver for the M5 CoreInk screen
fix an issue with inherited font size properties

Motivation and Context

I have a CoreInk, I want to display data.

How Has This Been Tested?

run a sample project to verify it

Screenshots

Types of changes

Improvement (non-breaking change that improves a feature, code or algorithm)
Bug fix (non-breaking change which fixes an issue with code or algorithm)
New feature (non-breaking change which adds functionality to code)
Breaking change (fix or feature that would cause existing functionality to change)
Config and build (change in the configuration and build system, has no impact on code or features)
Dependencies (update dependencies and changes associated, has no impact on code or features)
Unit Tests (add new Unit Test(s) or improved existing one(s), has no impact on code or features)
Documentation (changes or updates in the documentation, has no impact on code or features)

Checklist:

My code follows the code style of this project (only if there are changes in source code).
My changes require an update to the documentation (there are changes that require the docs website to be updated).
I have updated the documentation accordingly (the changes require an update on the docs in this repo).
I have read the CONTRIBUTING document.
I have tested everything locally and all new and existing tests passed (only if there are changes in source code).
I have added new tests to cover my changes.

xas · 2023-10-23T16:15:52Z

Should we separate code into 3 package nuget ?

EPaper
GDEW0154x
SSD168x

(on draft state, as I am waiting some specs confirmation from M5 team)

Ellerbach

Amazing start! And I love the fact that the interface makes sense :-) that's the first real different driver added here! So it's great to see! Few comments mainly about getting clarity for maintainability.
Also, you'll need to bump the minor version in the version file.

Ellerbach · 2023-10-24T16:00:57Z

devices/ePaper/Drivers/Gdew0154x/Gdew0154x.cs

+        {
+            _spiDevice = spiDevice ?? throw new ArgumentNullException(nameof(spiDevice));
+            _gpioController = gpioController ?? throw new ArgumentNullException(nameof(gpioController));
+


can you add a "_shouldDispose" to check if the gpio controller should be disposed? See the pattern here for example:

nanoFramework.IoT.Device/devices/A4988/A4988 .cs

Line 20 in 9ce639d

private readonly bool _shouldDispose;

In short, you can pass a null GpioController and you'll create it, also you'll dispose it in this case. If it's not null and the shouldDispose is true, you'll dispose it otherwise no.

Ellerbach · 2023-10-24T16:03:15Z

devices/ePaper/Drivers/Gdew0154x/Gdew0154x.cs

+            WaitMs(5);
+
+            SendCommand((byte)Command.DisplayRefresh);
+            WaitMs(500);


can you add a little comment on why to wait 5 ms and then 500 ms? Just a comment like "// 500 ms needed as per documentation page 5" will do it. So it makes it easier when maintaining and debugging all this

I did, but I think there are some non-sense between samples I found on M5 repo and the one provided by the screen manufacturer...

Ellerbach · 2023-10-24T16:06:24Z

devices/ePaper/Drivers/Gdew0154x/Gdew0154x.cs

+            }
+
+            SendCommand((byte)Command.PanelSetting);
+            SendData(0xdf);


can you point out where the initilization sequence is coming form? So it makes it easier for maintenance

Ellerbach · 2023-10-24T16:07:00Z

devices/ePaper/Drivers/Gdew0154x/Gdew0154x.cs

+        /// <inheritdoc/>
+        public virtual void PerformFullRefresh()
+        {
+            WaitMs(10);


why 10 milliseconds?

Ellerbach · 2023-10-24T16:07:34Z

devices/ePaper/Drivers/Gdew0154x/Gdew0154x.cs

+        public virtual void PowerOn()
+        {
+            HardwareReset();
+            WaitMs(1000);


few comments on the sequence will be greatly appreciated as well

Ellerbach · 2023-10-24T16:09:12Z

devices/ePaper/Drivers/Gdew0154x/Gdew0154x.cs

+            // make sure we are setting data/command pin to low (command mode)
+            _dataCommandPin.Write(PinValue.Low);
+
+            foreach (var b in command)


maybe for performance reasons, it could be faster to write all the bytes at once? As every time, the data is sent independently with a low and high on the chip select. So it's much slower than sending in a single write with a buffer.

Ellerbach · 2023-10-24T16:09:25Z

devices/ePaper/Drivers/Gdew0154x/Gdew0154x.cs

+            // set the data/command pin to high to indicate to the display we will be sending data
+            _dataCommandPin.Write(PinValue.High);
+
+            foreach (var @byte in data)


same comment as in the command one

Ellerbach · 2023-10-24T16:11:15Z

devices/ePaper/Drivers/Gdew0154x/Gdew0154x.cs

+        {
+            while (_busyPin.Read() != pinValue)
+            {
+                WaitMs(5);


if waiting is a matter of few milliseconds you can use the Thread.Spin. If it's indeed much more, then keep the Thread.Sleep

Ellerbach · 2023-10-24T16:11:39Z

devices/ePaper/Drivers/Gdew0154x/Gdew0154x.cs

+        {
+            while (_busyPin.Read() == PinValue.Low)
+            {
+                WaitMs(5);


same remark here as well

I did a little rewrite for the WaitBusy function as it was waiting indefinitely

Ellerbach · 2023-10-25T12:05:53Z

Should we separate code into 3 package nuget ?

I guess it's ok to have one only for the moment (it simplify a bit things), the code overhead is not that high

Ellerbach

better and better ;-)

Ellerbach · 2023-10-26T09:18:35Z

devices/ePaper/Drivers/Ssd168x/Ssd168x.cs

        {
-            while (_busyPin.Read() == PinValue.High)
+            int currentWait = 0;
+            while (currentWait < waitingTime && _busyPin.Read() == PinValue.High)


create a cancellation token, it's done for that. And you can use it in the loop as well to be non blocking

Is cancelling the wait on the busy pin a good idea here? If the waitingTime is set to 5 and after 25ms the display busy pin is still reading High, should we ignore that and move on with executing the rest of the commands knowing that they will not execute properly as the display is still stuck performing something else?

In my opinion, this will mask issues from the dev and might be harder to debug. If we must, I'd suggest throwing an exception so we have a clear signal to the caller/dev that the device has not responded in due time.

Something like (just did it from memory) this is much cleaner and you won't have to wait an extra 5 ms (which is ultimately going to be 20 ms) unnecessary. this shows as well how to set a default time.

public virtual bool WaitReady(CancellationToken waitingTime = default) { if (waitingTime == default) { // With a default time of 5 seconds as an example waitingTime = new CancellationTokenSource(5000); } while(!waitingTime.IsCancelledRequest && _busyPin.Read() == PinValue.High) { waitingTime.WaitHandle.WaitOne(5, true); } return !waitingTime.IsCancelledRequest; }

Ellerbach · 2023-10-26T09:19:58Z

devices/ePaper/Drivers/Gdew0154x/Gdew0154x.cs

+            int dataCommandPin,
+            int width,
+            int height,
+            GpioController gpioController,


you can even set this one as null and create it internally if not given.

MrCSharp22 · 2023-10-29T06:53:34Z

devices/ePaper/Drivers/Ssd168x/Ssd168x.cs

        {
-            while (_busyPin.Read() == PinValue.High)
+            int currentWait = 0;
+            while (currentWait < waitingTime && _busyPin.Read() == PinValue.High)


Is cancelling the wait on the busy pin a good idea here? If the waitingTime is set to 5 and after 25ms the display busy pin is still reading High, should we ignore that and move on with executing the rest of the commands knowing that they will not execute properly as the display is still stuck performing something else?

In my opinion, this will mask issues from the dev and might be harder to debug. If we must, I'd suggest throwing an exception so we have a clear signal to the caller/dev that the device has not responded in due time.

MrCSharp22 · 2023-10-29T07:03:12Z

devices/ePaper/Drivers/Ssd168x/Ssd168x.cs


            SendCommand((byte)Command.MasterActivation);
-            WaitReady();
+            WaitReady(_maxWaitingTime);


The return value of WaitReady is being ignored here. Would be good if it can be used to signal failure to the caller.

MrCSharp22 · 2023-10-29T07:23:53Z

devices/ePaper/Drivers/Gdew0154x/Gdew0154x.cs

+    /// <summary>
+    /// Base class for GDEW0154-based ePaper devices.
+    /// </summary>
+    public abstract class Gdew0154x : IEPaperDisplay


Can you rename the class to use the Controller model number instead which is 'JD79653A' ? GDEW0154x is an epaper product model number and not the controller name. As you can see in the other implemented driver, we're using SSD168x which is the controller name rather than the product number from GoodDisplay.

Also, please rename the namespace to match this.

MrCSharp22 · 2023-10-29T07:55:34Z

devices/ePaper/Drivers/Gdew0154x/Gdew0154x.cs

+        /// <param name="sleepMode">The sleep mode to use when powering down the display.</param>
+        public virtual void PowerDown(SleepMode sleepMode = SleepMode.Normal)
+        {
+            SendCommand(0x50);


Since you have the Command enum, these byte values should go there too.

MrCSharp22 · 2023-10-29T07:56:19Z

devices/ePaper/Drivers/Gdew0154x/Gdew0154x.cs

+                HardwareReset();
+            }
+
+            SendCommand(0x50);


Same here. Please move the byte value to Command enum.

xas · 2023-11-02T11:05:12Z

For the busy pin question, I am more on the solution of warn the developer, but not block him
The current implementation of WaitReady is a blocking thread, so if something happen on the screen side, the all app is blocked.

I think it should be the developer role to decide what to do if after a while the busy pin is still up.

I am also against raising an exception as you could crash the developer's app.

I am more on a warning policy: "hey be careful, seems there are still work on the screen, but continue with the rest"

xas · 2023-11-02T11:07:04Z

BTW, is it really necessary Iot.Device.EPaper.Drivers.JD79653A.JD79653A ?

I don't think a class and a namespace should share a same name. Why the class Iot.Device.EPaper.Drivers.JD79653Ashould be wrong ?

Ellerbach · 2023-11-02T13:24:25Z

BTW, is it really necessary Iot.Device.EPaper.Drivers.JD79653A.JD79653A ?

no it's not. Fine to use directly the Driver namespace!

MrCSharp22 · 2023-11-06T11:23:06Z

BTW, is it really necessary Iot.Device.EPaper.Drivers.JD79653A.JD79653A ?

Not necessary, no. What I was thinking about is how JD79653A seems to be part of a family of drivers. I found references to another epd driver JD796**32A** so my thinking is that the full namespace of the class becomes Iot.Device.EPaper.Drivers.JD796xx.JD79653A so if later on we add JD796**32A**, it will live there.

* typo

* add "shouldDispose" property * fix data sending protocol * clean up time delays * improve data write buffering * use of SpinWait for short time * add a time limit to "WaitReady" to avoid infinite wait * add a deep sleep power state

* namespace driver renaming * fix missing command map * allow gpioController parameter to be null

Ellerbach

Couple of small details. Great to see the PR now real review :-)

Ellerbach · 2023-11-20T11:18:19Z

devices/ePaper/Drivers/JD796xx/JD79653A.cs

+    /// <summary>
+    /// Base class for GDEW0154-based ePaper devices.
+    /// </summary>
+    public abstract class JD79653A : IEPaperDisplay


Technically the name should be Jd79653A to follow our guidelines. Same for the namespace Jd796xx

Ellerbach · 2023-11-20T11:30:52Z

devices/ePaper/Drivers/Ssd168x/Ssd168x.cs

    /// </summary>
    public abstract class Ssd168x : IEPaperDisplay
    {
+        private readonly int _maxWaitingTime = 500;


why not a const?

I want to leave the possibility that become a configurable property when instantiating it.

Ellerbach

Thanks, just a little detail on the wait function using cancellation token

Ellerbach · 2023-11-28T09:44:18Z

devices/ePaper/Drivers/Ssd168x/Ssd168x.cs

        {
-            while (_busyPin.Read() == PinValue.High)
+            int currentWait = 0;
+            while (currentWait < waitingTime && _busyPin.Read() == PinValue.High)


Something like (just did it from memory) this is much cleaner and you won't have to wait an extra 5 ms (which is ultimately going to be 20 ms) unnecessary. this shows as well how to set a default time.

public virtual bool WaitReady(CancellationToken waitingTime = default) { if (waitingTime == default) { // With a default time of 5 seconds as an example waitingTime = new CancellationTokenSource(5000); } while(!waitingTime.IsCancelledRequest && _busyPin.Read() == PinValue.High) { waitingTime.WaitHandle.WaitOne(5, true); } return !waitingTime.IsCancelledRequest; }

Ellerbach

Looks all good now! Thanks!

Ellerbach · 2024-01-05T08:30:35Z

@xas you still have to fix the versions between the nuget and the nfproj and what's missing. See the build result.

… into develop

Ellerbach

I think we're good to go :-) HUGE thanks for the work on this contribution! Great to see this additional epaper screen!

nfbot added Type: bug Something isn't working Type: enhancement New feature or request labels Oct 23, 2023

xas force-pushed the develop branch from 97742d4 to 448b636 Compare October 23, 2023 16:17

Ellerbach reviewed Oct 24, 2023

View reviewed changes

Ellerbach reviewed Oct 26, 2023

View reviewed changes

xas mentioned this pull request Oct 27, 2023

add M5 CoreInk project nanoframework/nanoFramework.M5Stack#304

Merged

14 tasks

MrCSharp22 suggested changes Oct 29, 2023

View reviewed changes

xas added 4 commits November 6, 2023 16:04

feat: add GDEW0154x e-paper driver

45cbffe

feat: add GDEW0154x e-paper driver

4ba4eba

* typo

fix: reviews

8d53166

* add "shouldDispose" property * fix data sending protocol * clean up time delays * improve data write buffering * use of SpinWait for short time * add a time limit to "WaitReady" to avoid infinite wait * add a deep sleep power state

fix: reviews

f4d250a

* namespace driver renaming * fix missing command map * allow gpioController parameter to be null

xas force-pushed the develop branch from 3ee25c4 to f4d250a Compare November 6, 2023 15:04

xas added 4 commits November 7, 2023 09:34

feat: PerformRefresh functions returns boolean for state refresh

caf865e

fix: source for screen initialization sequence

a7e0cfb

fix: conflicts

4f3ca61

Merge branch 'develop' into develop

6e5aae6

xas marked this pull request as ready for review November 20, 2023 08:05

Ellerbach reviewed Nov 20, 2023

View reviewed changes

Ellerbach and others added 2 commits November 28, 2023 12:19

Merge branch 'develop' into develop

7e50fa5

fix: Jd796 typo

ad7f8c2

xas force-pushed the develop branch from 111d6ed to ad7f8c2 Compare November 28, 2023 09:28

Ellerbach reviewed Nov 28, 2023

View reviewed changes

josesimoes force-pushed the develop branch 2 times, most recently from 9de53d1 to c8ac54b Compare December 11, 2023 13:24

xas added 2 commits January 4, 2024 15:55

Merge branch 'develop' into develop

7ae1987

fix: MR reviews for ePaper project

aa7b578

Ellerbach approved these changes Jan 5, 2024

View reviewed changes

Merge branch 'develop' into develop

a283799

xas added 6 commits January 9, 2024 16:08

fix: update nuspec file according to nfproj file

c3f7144

Merge branch 'develop' into develop

27a5668

Merge branch 'develop' of github.com-xas:xas/nanoFramework.IoT.Device…

00dfd1c

… into develop

fix: update nuspec file according to nfproj file

eb86cc9

fix: update nuspec file according to nfproj file

448b4fd

Merge branch 'develop' into develop

1568aee

Ellerbach approved these changes Jan 11, 2024

View reviewed changes

Ellerbach merged commit cb8d99a into nanoframework:develop Jan 11, 2024

Uh oh!

add GDEW0154x e-paper driver #902

add GDEW0154x e-paper driver #902

Uh oh!

Conversation

xas commented Oct 23, 2023

Description

Motivation and Context

How Has This Been Tested?

Screenshots

Types of changes

Checklist:

Uh oh!

xas commented Oct 23, 2023

Uh oh!

Ellerbach 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!

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!

Ellerbach commented Oct 25, 2023

Uh oh!

Ellerbach 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!

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!

xas commented Nov 2, 2023

Uh oh!

xas commented Nov 2, 2023

Uh oh!

Ellerbach commented Nov 2, 2023

Uh oh!

MrCSharp22 commented Nov 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ellerbach 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

MrCSharp22 commented Nov 6, 2023 •

edited

Loading