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

Set pixels as image, add show_bitmap_1d(), show_text() #121

Merged
merged 21 commits into from
May 7, 2021

Conversation

graeme-winter
Copy link
Contributor

Preamble

I don't know how you folks feel about PR's - if this is not your game, I will not be offended if I see a close with no comments.

Proposed API extensions

Show images

Show images using a set_pixels method:

image = bytearray(0 for j in range(width * height))
picoscroll.set_pixels(image)

seems convenient for cases where you want to redraw the image from one "frame" to the next, means all of the pixel value copying is done in C not Python/

Scroll bitmaps

Scroll bitmaps (one dimensional, e.g. text) using a show_bitmap_1d method:

bitmap = bytearray(j for j in range 127)
for offset in range(-17, 127):
    picoscroll.show_bitmap_1d(bitmap, 16, offset)
    picoscroll.update()

Detailed illustration in this gist

Motivation for second one: pico scroll did not offer a simple method to actually scroll text across the display, and I feel that this is pretty fundamental so decided to offer one. I wondered about adding the code to render the text to a bitmap to the API but decided on balance to keep things simple, as fonts are something people hold strong opinions about.

End product:

https://youtu.be/XIvKc523NwM

Will welcome any feedback here, I tried to stick to the "house style"

@graeme-winter
Copy link
Contributor Author

Ugh, I seem to have mixed tabs and spaces, sorry.

If you have some cleanup I should run, happy to do this (I did not see a developer guide) - am used to running black on Python code or clang-format on C/C++ -> don't usually pay too much attention to the type of white space.

@graeme-winter
Copy link
Contributor Author

🤔

Having just displayed

x128 = bytearray(range(128))
for offset in range(-17, len(hello)):
    scroll.show_bitmap_1d(hello, 16, offset)
    scroll.update()
    time.sleep(0.1)

I think I have this upside down, so my font map is upside down 🙄

Guess I should have done that first -> will revise.

@graeme-winter
Copy link
Contributor Author

Turns out this is probably largely redundant having looked at

https://github.com/pimoroni/scroll-phat-hd

which would probably be a better API if ported (though still does not allow for a scrolling view across a larger buffer). Does contain a useful 5x7 font though (found in HackSpace issue 41)

@Gadgetoid Gadgetoid added the enhancement New feature or request label Apr 12, 2021
@Gadgetoid
Copy link
Member

PR's - especially ones this thorough and well considered - are always welcome. Thank you! I'll have to give this a try, but I like what you're doing here. Your premise is solid - with "scroll" in the product name but not demonstrated in the software things feel a bit silly.

It could be well worth baking the 5x7 font into the C++ library and adding functions for rendering text across an arbitrary length buffer. There are definitely differences of opinion when it comes to fonts, but I guess having one baked-in wouldn't prevent anyone from ignoring it and rolling their own.

Given the relatively small memory overhead, maybe we could add a "scroll_text" function that just takes a text string, a speed and a loop count and just does the rest for you. It may be possible to render the text even on the fly and avoid a full-sized buffer altogether.

@graeme-winter
Copy link
Contributor Author

@Gadgetoid thanks for the positive feedback - am working on adding in a C implementation of the font rendering I used in the Python code, so should see a commit added to the PR soon. Adding a scroll_text function based on that would be pretty straightforward...

Along these lines, I guess something for showing text (e.g. 14°C or whatever) in a non-scrolling manner would also be useful. May as well get all this done in one go.

@graeme-winter graeme-winter changed the title Set pixels as image, add show_bitmap_1d() Set pixels as image, add show_bitmap_1d(), show_text() Apr 15, 2021
@graeme-winter
Copy link
Contributor Author

I have added show_text() now, some doc strings to go with this. This is the full character set:

fontmap

I welcome any further suggestions.

I would like to run the whole thing through e.g. clang-format to clean up the indentation / white space etc. but holding off in case you have tools for this.

@graeme-winter
Copy link
Contributor Author

@Gadgetoid I have added scroll_text with this API:

text = "Hello, world!"
brightness = 8
delay_ms = 100
scroll.scroll_text(text, brightness, delay_ms)

It's not set to loop, as I figured you could do that externally if you wanted. It will scroll the text for an offset of -17 to 6 x len(text) i.e. coming from right, towards left, invisible to invisible. I also have a show_text() method added which is designed to show "one frame" though as you suggest could be done with on-the-fly rendering. If I get a chance, I will refactor that, as it's currently a little inefficient if you use show_text() in a loop as I am rendering bytes which will never be seen.

However, this is probably in a good place for review now.

Also fixed space assignment in font render
@graeme-winter
Copy link
Contributor Author

For info, show_text() now only renders what will be shown so the static buffer is one letter + spaces bigger than the view.

0-5 dice 1-6
2 x rectangle
2 x square
smile / neutral / sad / confused
top, bottom lines
@Gadgetoid
Copy link
Member

I would like to run the whole thing through e.g. clang-format to clean up the indentation / white space etc. but holding off in case you have tools for this.

I wanted to make this part of the GitHub workflow so we could enforce some consistency on the whole codebase... I know line endings have gone all awry at least. An initial tidy would probably be part of the commits adding that change and probably worth doing separately.

Thanks for all the effort! I'll have a play with this now.

@Gadgetoid
Copy link
Member

D'oh I cannot for the life of me find a Pico Scroll Pack so I've put one on order and will get to this ASAP!

@Gadgetoid Gadgetoid self-requested a review April 20, 2021 08:51
@graeme-winter
Copy link
Contributor Author

D'oh I cannot for the life of me find a Pico Scroll Pack so I've put one on order and will get to this ASAP!

🤣

@Gadgetoid
Copy link
Member

Okay having got this up and running, it works great but it should be implemented in the C++ libraries so both C++ Pico SDK and MicroPython users can use it.

I've done that for scroll_text in this commit - Gadgetoid@f851151

I've stubbed out picoscroll_show_text pending the same treatment.

This, in theory, should have no appreciable difference to Python users but makes this good stuff available to everyone else too.

@Gadgetoid
Copy link
Member

Okay that commit is stale now, I've force pushed to Gadgetoid@b3798cd which isn't quite so broken!

@Gadgetoid
Copy link
Member

Gadgetoid commented Apr 21, 2021

Aaand again. I might have got carried away here - Gadgetoid@cc3e997

Crux of this is making everything use the same underlying utility function - set_bitmap_1d so there's no unnecessary code repetition,

Brings show_text (now set_text), show_bitmap_1d (now set_bitmap_1d) and set_pixels into the fold, too.

@graeme-winter
Copy link
Contributor Author

I'll checkout your version a bit later... happy to see merged with C library

Move scroll_text into the C++ library and make it support std::string.

Move show_bitmap_1d to set_bitmap_1d in the C++ library. Use it as the basis for show_text and scroll_text.

Change show_text to set_text since it does not implicitly show the result.

Add a new pico-scroll demo to show off the scrolling text functionality.
@graeme-winter
Copy link
Contributor Author

Picked, built & run - seems to do the same things 🙂

Having in C++ API makes more sense but was not something I was in a position to test.

How do you want to proceed with this now? Am happy to push the commit to this PR?

@Gadgetoid
Copy link
Member

Sure, you can push it to this PR- any way works for me.

I had the scrolling text running on my desk for a day or so, rock solid. I'll have to see if it's pin compatible with 👀 pico wireless pack 👀.

I merged the Matrix11x7 driver today and couldn't help but think all this code should probably be ported over. Will have to add that to The List. 😆

@graeme-winter
Copy link
Contributor Author

Sure, you can push it to this PR- any way works for me.

I had the scrolling text running on my desk for a day or so, rock solid. I'll have to see if it's pin compatible with 👀 pico wireless pack 👀.

I merged the Matrix11x7 driver today and couldn't help but think all this code should probably be ported over. Will have to add that to The List. 😆

It's pushed - happy to see this be recycled as much as anyone likes! I only have the scroll top though

@Gadgetoid Gadgetoid merged commit 5a60c47 into pimoroni:main May 7, 2021
@Gadgetoid
Copy link
Member

Woohoo! Merged. Will get this out in a Python release ASAP!

@graeme-winter
Copy link
Contributor Author

Woohoo! Merged. Will get this out in a Python release ASAP!

\o/

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

Successfully merging this pull request may close these issues.

None yet

2 participants