Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

epd2in66b: Waveshare 2.66inch E-Paper Display Module (B) for Raspberry Pi Pico #673

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from

Conversation

trichner
Copy link
Contributor

@trichner trichner commented Apr 20, 2024

A simple driver for the Waveshare 2.66in EPD for the Pico. Heavily inspired by the official python implementation and its magic timing.

See also: https://www.waveshare.com/Pico-ePaper-2.66-B.htm

Using machine package currently breaks the build.
How should I go about removing the machine package from the unit tests?

Example looks like this:
image

@deadprogram
Copy link
Member

In an optimal scenario, we would add an spi test capabilty similar to https://github.com/tinygo-org/drivers/blob/release/tester/i2c.go

In the meantime, using the drivers.SPI interface instead of machine.SPI can also help.

Comment on lines 41 to 42
width int16
height int16

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these need to be in the struct since they are not being changed. Constants could be used directly, but I would rename them to something more unique (eg displayWidth) to avoid unintentional shadowing.

Comment on lines 134 to 140
func set(buf []byte, bytePos, bitPos int, v bool) {
if v {
buf[bytePos] |= 0x1 << bitPos
} else {
buf[bytePos] &^= 0x1 << bitPos
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe split this function into two: set and clear so you don't have to have bool argument and if. It should be a bit faster that way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good point. Done.

@@ -19,7 +19,7 @@ rwildcard=$(foreach d,$(wildcard $1*),$(call rwildcard,$d/,$2) $(filter $(subst
# Recursively find all *_test.go files from cwd & reduce to unique dir names
HAS_TESTS = $(sort $(dir $(call rwildcard,,*_test.go)))
# Exclude anything we explicitly don't want to test for whatever reason
EXCLUDE_TESTS = image
EXCLUDE_TESTS = image waveshare-epd/epd2in66b
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively I remove the tests for now, though I found them quite useful for myself.

@trichner trichner force-pushed the tr/epd2in66b branch 2 times, most recently from f5d785e to 738cd08 Compare April 27, 2024 15:44
@deadprogram
Copy link
Member

@trichner
Copy link
Contributor Author

Looks like it is missing the example? https://github.com/tinygo-org/drivers/actions/runs/8860923503/job/24332227160?pr=673#step:8:308

Too much copy paste, should be fixed. Sorry about that.

@aykevl
Copy link
Member

aykevl commented Apr 28, 2024

Why is this driver specific to the pico? It looks like a regular driver to me that should work on any chip?

@trichner
Copy link
Contributor Author

trichner commented Apr 28, 2024

Why is this driver specific to the pico? It looks like a regular driver to me that should work on any chip?

The driver 'should' already work with any other chip. The pins & SPI is all configurable.
I only have the pico HAT to test currently though: https://www.waveshare.com/pico-epaper-2.66.htm

If this looks too specific, I can also remove the pico HAT specific defaults.

What would you prefer? Either works for me.

On a side note: I did not manage to figure out whether this is a specific waveshare driver chip or something more generic.

@deadprogram
Copy link
Member

If this looks too specific, I can also remove the pico HAT specific defaults.

It would be better to make them more generic.

To make life easier for devs, you could also add a file protected by build tag, with the specific values for the board you are using. That way the "default" case for your board "just works".

@trichner what do you think?

@trichner
Copy link
Contributor Author

trichner commented May 3, 2024

If this looks too specific, I can also remove the pico HAT specific defaults.

It would be better to make them more generic.

To make life easier for devs, you could also add a file protected by build tag, with the specific values for the board you are using. That way the "default" case for your board "just works".

@trichner what do you think?

Sounds good! I gave it a try with build flags, was this what you had in mind?

Comment on lines +30 to +31
// in case you have a Pico module, you can directly use
// dev, err := epd2in66b.NewPicoModule()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've kept the example for the generic setup with this hint for the specific pico use case.

@@ -0,0 +1,34 @@
//go:build pico
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

obviously only works for picos ;)

@trichner
Copy link
Contributor Author

trichner commented May 7, 2024

@deadprogram let me know if anything else is missing or needs change.

@trichner trichner requested a review from deadprogram May 18, 2024 13:48
@trichner
Copy link
Contributor Author

Alright, all cleaned up!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants