Add example on docs and /examples folder #66

Slushee-a · 2023-07-08T12:32:59Z

Why

As mentioned in #65, two improvements that could be made were adding another example project and expanding documentation on set_pixels()

Changes included

Adds another example to the /examples folder.
- Made for the esp32-c3
- Using the ili9486 display with SPI
- Shows a basic example of howset_pixels() can be used
Adds an example to the set_pixels() on the docs

examples/spi-ili9486-esp32-c3/src/main.rs

rfuest · 2023-07-08T14:07:28Z

examples/spi-ili9486-esp32-c3/src/main.rs

+    // Turn SPRITE into an array of pixels in the right colorspace
+    let sprite: [Rgb565; 256] = SPRITE.map(|(r, g, b)| Rgb565::new(r, g, b));
+
+    // Draw the 16*16 sprite on the top left corner of the screen
+    display.set_pixels(0, 0, 15, 15, sprite).unwrap();


This is pretty inefficient, because the sprite array needs to be build in RAM. I'm biased and would simply use embedded-graphics to draw an image, but if you want to demonstrate set_pixels I would suggest to change the SPRITE constant to be SPRITE: [Rgb565; 256] instead.

In addition to that the SPRITE constant seems to use values between 0 and 255 while Rgb565 expects 5 or 6 bit values depending on the channel.

I did want to demonstrate how map can be used to turn a regular array into an array of RgbXYZ colors, but it does make more sense to just use embedded-graphics I will change this.

I will also add more to the example, drawing a smily face or something similar to demonstrate the use of embedded-graphics instead

If you want to make this an example of using set_pixels I'm OK with that, but the example should make it clear that there are other options available. For more memory constrained targets the array in RAM could be a problem for larger images.

You don't need an array, because set_pixels takes an iter: display.set_pixels(0, 0, 15, 15, SPRITE.map(|(r, g, b)| Rgb565::new(r, g, b))).unwrap(); should also work, without the memory overhead.

I opted to use const SPRITE: [Rgb565; 256] = [Rgb565::RED, Rgb565::ORANGE ... instead with fill_contiguous, since I just wanted to demonstrate how it is possible to use a custom sprite. If this is okay you can resolve the conversation. Otherwise do suggest something

examples/spi-ili9486-esp32-c3/src/main.rs

rfuest · 2023-07-08T14:23:55Z

src/lib.rs

@@ -179,6 +179,23 @@ where
    /// * `ey` - y coordinate end
    /// * `colors` - anything that can provide `IntoIterator<Item = u16>` to iterate over pixel data
    ///
+    /// # Example


If you want to improve the docs for set_pixel further:

One thing that is important to document IMO is the behavior of set_pixels if the colors iterator contains more pixels than required by the rectangle bounds. If I remember correctly there is no bounds checking and the drawing operation would just wrap back to the top left of the rectangle.

This is different to embedded-graphics's similar fill_contiguous method and should be highlighted. Because this makes it impossible to just use something like display.set_pixel(0, 0, 10, 10, core::iter::repeat(Rgb565::RED)), which works in e-g.

This is different to embedded-graphics's similar fill_contiguous method and should be highlighted

I added a # Warning here » mentioning that using fill_contiguous instead is recommended. Does this sound good or should I highlight that in another way?

I would move it to above the argument list and rephrase it a bit. It's quite important to know the behavior of and IMO it's harder to notice the warning if it is below the example.

Instead of a warning I would just describe the behavior of this method. A rough draft of a description:

No bounds checking is performed on the colors iterator, which can lead to unexpected behavior if the iterator returns more or less color values than the number of pixels in the given update region. Use fill_contiguous from embedded-graphics as an alternative with bounds checking.

One note here is that set_pixels is essentially a driver primitive. It's not meant to be used directly and is exposed so frameworks other than embedded-graphics can be used.

Slushee-a · 2023-07-08T15:29:15Z

Changes included on the latest commit:

Removed map example from set_pixels, swapped directly with an Rgb666 array instead
Removed map + set_pixels segment from spi-ili9486-esp32-c3 example in favor of fill_contiguous with an Rgb565 array
Added an example drawing of a basic smiley face on the spi-ili9486-esp32-c3 example
Fix typo
Fix dependency usage

examples/spi-ili9486-esp32-c3/src/main.rs

rfuest · 2023-07-08T15:50:30Z

examples/spi-ili9486-esp32-c3/src/main.rs

+    display.clear(Rgb565::BLACK).unwrap();
+
+    // draw the 16*16 sprite on the top left corner of the screen
+    display.fill_contiguous(&sprite_rectangle, SPRITE).unwrap();


We don't normally recommend to use the methods in DrawTarget directly in user code, but I'm not sure what the best option is in this case. The "embedded-graphics way" of drawing an image would be to use ImageRaw and Image, but that wouldn't work with the way you initialize the SPRITE constant, because ImageRaw expects a byte array. Perhaps we just leave it is now.

I think it is best to make the examples as 'correct' on the usage as possible, since they're made to demonstrate how things are intended to be done.
Maybe this example can be left as a 'possibility', but I think an example using ImageRaw and Image is important, since drawing custom sprites to the screen is probably a common use for the crate.
To be fair though, this is done by embedded-graphics so maybe it's out of scope.

I would either remove the image drawing from this example or change it to something that is easy to use. Drawing pixels from an array that is manually added to the code isn't very common IMO.

Using the tinybmp instead of ImageRaw would make it much easier for a user to generate the necessary image data, because they can just export a BMP file from an image editor like Gimp. The BMP format does also natively support RGB565 colors, which makes it a good fit for the used display. See the tinybmp for an example how to use it: https://github.com/embedded-graphics/tinybmp#draw-a-bmp-image-to-an-embedded-graphics-draw-target

Regardless of whether we decide to remove the image drawing or not, a link to the embedded-graphics example would be a good idea to get an overview of the possible drawing operations: https://docs.rs/embedded-graphics/latest/embedded_graphics/examples/index.html

examples/spi-ili9486-esp32-c3/src/main.rs

rfuest · 2023-07-08T16:09:24Z

src/lib.rs

@@ -179,6 +179,23 @@ where
    /// * `ey` - y coordinate end
    /// * `colors` - anything that can provide `IntoIterator<Item = u16>` to iterate over pixel data
    ///
+    /// # Example


I would move it to above the argument list and rephrase it a bit. It's quite important to know the behavior of and IMO it's harder to notice the warning if it is below the example.

Instead of a warning I would just describe the behavior of this method. A rough draft of a description:

No bounds checking is performed on the colors iterator, which can lead to unexpected behavior if the iterator returns more or less color values than the number of pixels in the given update region. Use fill_contiguous from embedded-graphics as an alternative with bounds checking.

Slushee-a · 2023-07-08T16:41:28Z

Changes included on the latest commit:

Switched away from Rectangle::with_corners in favor of Rectangle::new
Move smiley face drawing from main to a separate function
Remove outdated comment (Sorry!)
Explain set_pixels() behavior instead of warning

almindor

LGTM but I'll wait for @rfuest to +1 as well

almindor · 2023-07-08T17:44:02Z

src/lib.rs

@@ -179,6 +179,23 @@ where
    /// * `ey` - y coordinate end
    /// * `colors` - anything that can provide `IntoIterator<Item = u16>` to iterate over pixel data
    ///
+    /// # Example


One note here is that set_pixels is essentially a driver primitive. It's not meant to be used directly and is exposed so frameworks other than embedded-graphics can be used.

examples/spi-ili9486-esp32-c3/src/main.rs

rfuest · 2023-07-09T17:22:01Z

examples/spi-ili9486-esp32-c3/src/main.rs

+    display.clear(Rgb565::BLACK).unwrap();
+
+    // draw the 16*16 sprite on the top left corner of the screen
+    display.fill_contiguous(&sprite_rectangle, SPRITE).unwrap();


I would either remove the image drawing from this example or change it to something that is easy to use. Drawing pixels from an array that is manually added to the code isn't very common IMO.

Using the tinybmp instead of ImageRaw would make it much easier for a user to generate the necessary image data, because they can just export a BMP file from an image editor like Gimp. The BMP format does also natively support RGB565 colors, which makes it a good fit for the used display. See the tinybmp for an example how to use it: https://github.com/embedded-graphics/tinybmp#draw-a-bmp-image-to-an-embedded-graphics-draw-target

Regardless of whether we decide to remove the image drawing or not, a link to the embedded-graphics example would be a good idea to get an overview of the possible drawing operations: https://docs.rs/embedded-graphics/latest/embedded_graphics/examples/index.html

src/lib.rs

Slushee-a · 2023-07-09T18:42:37Z

I do agree with the fact that the fill_contiguous part of the example should be removed, since drawing basic shapes is already enough for the example.

Adding a link to the embedded-graphics examples is a great idea. Should it be added to the docs homepage or to the README?

rfuest

Looks good, except for a missing word in one of the comments. Thanks for your efforts to improve the examples and docs. It's sometimes hard to know what is missing if you are already very familiar with the create.

src/lib.rs

rfuest · 2023-07-10T01:22:26Z

Adding a link to the embedded-graphics examples is a great idea. Should it be added to the docs homepage or to the README?

Both locations would make sense, some users might learn about the crate via the README while others read the docs first. The README already contains a reference to e-g with this sentence: "embedded-graphics-core is used to provide the drawing API.", but that isn't very useful for users of the crate IMO. The distinction between embedded-graphics-core and embedded-graphics should be considered an implementation detail and not something that is important for users. A short description what e-g is and links to the relevant docs would be more useful than a link to e-g-core.

Co-authored-by: Ralf Fuest <[email protected]>

Add spi-esp32 example

7be4f95

Slushee-a requested a review from almindor as a code owner July 8, 2023 12:32

rfuest requested changes Jul 8, 2023

View reviewed changes

rfuest reviewed Jul 8, 2023

View reviewed changes

examples/spi-ili9486-esp32-c3/src/main.rs Outdated Show resolved Hide resolved

rfuest reviewed Jul 8, 2023

View reviewed changes

Expand on esp32 example and set_pixels

e611757

rfuest requested changes Jul 8, 2023

View reviewed changes

Clean up spi-esp32 example and draw_pixels warning

fc7c363

Clarify behavior of set_pixels

dba549d

almindor approved these changes Jul 8, 2023

View reviewed changes

Clarify behavior of set_pixels

9a72501

rfuest requested changes Jul 9, 2023

View reviewed changes

Remove image drawing example, clear up docs

22f64ec

rfuest approved these changes Jul 10, 2023

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

Add missing word

1449999

Co-authored-by: Ralf Fuest <[email protected]>

rfuest merged commit d3c3347 into almindor:master Jul 10, 2023
6 checks passed

rfuest mentioned this pull request Jul 12, 2023

Final part of GC9A01 display cut off #67

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add example on docs and /examples folder #66

Add example on docs and /examples folder #66

Slushee-a commented Jul 8, 2023

rfuest Jul 8, 2023

Slushee-a Jul 8, 2023 •

edited

Loading

rfuest Jul 8, 2023

Slushee-a Jul 8, 2023 •

edited

Loading

rfuest Jul 8, 2023

Slushee-a Jul 8, 2023

rfuest Jul 8, 2023

almindor Jul 8, 2023

Slushee-a commented Jul 8, 2023

rfuest Jul 8, 2023

Slushee-a Jul 8, 2023

rfuest Jul 9, 2023

rfuest Jul 8, 2023

Slushee-a commented Jul 8, 2023 •

edited

Loading

almindor left a comment

almindor Jul 8, 2023

rfuest Jul 9, 2023

Slushee-a commented Jul 9, 2023 •

edited

Loading

rfuest left a comment

rfuest commented Jul 10, 2023

Add example on docs and /examples folder #66

Add example on docs and /examples folder #66

Conversation

Slushee-a commented Jul 8, 2023

Why

Changes included

Choose a reason for hiding this comment

Slushee-a Jul 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Slushee-a Jul 8, 2023 • edited Loading

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

Slushee-a commented Jul 8, 2023

Changes included on the latest commit:

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

Slushee-a commented Jul 8, 2023 • edited Loading

Changes included on the latest commit:

almindor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Slushee-a commented Jul 9, 2023 • edited Loading

rfuest left a comment

Choose a reason for hiding this comment

rfuest commented Jul 10, 2023

Slushee-a Jul 8, 2023 •

edited

Loading

Slushee-a Jul 8, 2023 •

edited

Loading

Slushee-a commented Jul 8, 2023 •

edited

Loading

Slushee-a commented Jul 9, 2023 •

edited

Loading