jasonacox / TM1637TinyDisplay

Arduino library to display numbers and text on a 4 and 6 digit 7-segment TM1637 display modules.
GNU Lesser General Public License v3.0
69 stars 19 forks source link

ENHANCEMENT: Non-blocking animations/scrolling #22

Open hackerceo opened 1 year ago

hackerceo commented 1 year ago

Hi, I am looking for this functionality in a non-blocking design. Is there any plans to do this? Is there interest if I do this and issue a pull request?

Thanks, Nick

Kravatox commented 1 year ago

Thanks for the answer, I'm a beginner with the Arduino code and I will not be able to help you, sorry. I have another question, in my code I use two display, one with 4 digits and another with 6 digits, and when I include TM1637TinyDisplay.h library and TM1637TinyDisplay6.h I have this error : "warning: "MAXDIGITS" redefined" and "warning: "FRAMES" redefined". Is there any solution to use both library the same time ?

Le 30 août 2022 à 17:10, Nick Benik @.***> a écrit :

Hi, I am looking for this functionality in a non-blocking design. Is there any plans to do this? Is there interest if I do this and issue a pull request?

Thanks, Nick

— Reply to this email directly, view it on GitHub https://github.com/jasonacox/TM1637TinyDisplay/issues/22, or unsubscribe https://github.com/notifications/unsubscribe-auth/A2QLVBQEEQXCKJ5BBJ6X4STV3YP5LANCNFSM6AAAAAAQAQE3GI. You are receiving this because you are subscribed to this thread.

jasonacox commented 1 year ago

Hi @hackerceo - Thanks for opening this issue. Great question and yes, I would be happy to accept a PR that gives us that enhancement! 🙏

@Kravatox I'll move your question to a new issue as it is not related to animation/scrolling: See #23

Frtrillo commented 1 year ago

Hello, at least non-blocking scrolling text would be very useful IMO

hackerceo commented 1 year ago

I plan to work on this Sept-Oct. It should play animations AND scroll text in a non-blocking way.

jasonacox commented 1 year ago

Thanks for the PR here https://github.com/jasonacox/TM1637TinyDisplay/pull/24 ! I can't wait to use this for my projects.

I have one minor change suggestion for API consistency:

Can we change the parameters for scrollString() and startAnimation() where we put content payload first and put bool usePROGMEM = false at the end with a default to false? This would align with the rest of the function. We could even add scrollString_P() and startAnimation_P() as alias functions that call their main functions with usePROGMEM = true.

Seems like that would help with consistency, but I could be convinced otherwise. What do you think?

  void startAnimation(const uint8_t (*data)[4], unsigned int frames = 0, unsigned int ms = 10, bool usePROGMEM = false );
  void scrollString(const char s[], unsigned int ms = DEFAULT_SCROLL_DELAY, bool usePROGMEM = false);

And thanks, @hackerceo - brilliant work!

hackerceo commented 1 year ago

One last thing. What do you think about renaming "scrollString" function to "startStringScroll"? I think having the functions which start non-blocking operations begin their name with "start" makes it a little more clear that those functions only start the process. One could assume "scrollString" will handle everything for you.

On Sun, Sep 18, 2022, 1:54 AM Jason Cox @.***> wrote:

Thanks for the PR here #24 https://github.com/jasonacox/TM1637TinyDisplay/pull/24 ! I can't wait to use this for my projects.

I have one minor change suggestion for API consistency:

Can we change the parameters for scrollString() and startAnimation() where we put content payload first and put bool usePROGMEM = false at the end with a default to false? This would align with the rest of the function. We could even add scrollString_P() and startAnimation_P() as alias functions that call their main functions with usePROGMEM = true.

Seems like that would help with consistency, but I could be convinced otherwise. What do you think?

void startAnimation(const uint8_t (*data)[4], unsigned int frames = 0, unsigned int ms = 10, bool usePROGMEM = false ); void scrollString(const char s[], unsigned int ms = DEFAULT_SCROLL_DELAY, bool usePROGMEM = false);

— Reply to this email directly, view it on GitHub https://github.com/jasonacox/TM1637TinyDisplay/issues/22#issuecomment-1250198425, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAWCKMUVECH7Y4WO3C4ODZLV62VBBANCNFSM6AAAAAAQAQE3GI . You are receiving this because you were mentioned.Message ID: @.***>

jasonacox commented 1 year ago

I like it! Great thinking @hackerceo ... And it is consistent with startAnimation(). Do you want to make these changes or should I? I can merge and update or can wait to merge until after you commit the changes to the same PR.

hackerceo commented 1 year ago

I'll make the changes in a few hours.

On Sun, Sep 18, 2022, 4:11 PM Jason Cox @.***> wrote:

I like it! Great thinking @hackerceo https://github.com/hackerceo ... And it is consistent with startAnimation(). Do you want to make these changes or should I? I can merge and update or can wait to merge until after you commit the changes to the same PR.

— Reply to this email directly, view it on GitHub https://github.com/jasonacox/TM1637TinyDisplay/issues/22#issuecomment-1250378439, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAWCKMTEQR5FDWQC3EAF2ILV65ZOFANCNFSM6AAAAAAQAQE3GI . You are receiving this because you were mentioned.Message ID: @.***>

hackerceo commented 1 year ago

@Frtrillo you should be happy as non-blocking animation/scrolling functionality is now added to this library!

jasonacox commented 1 year ago

I'll release this as v1.7.0 for the Arduino library and add a TODO to port this to the 6-digit display as well.

Thanks for this great enhancement @hackerceo !

hackerceo commented 1 year ago

Sounds great! I think if we can mention non-blocking animations that would attract adoption and use of the library. I think this is the only TM1637 library that does that.

On Mon, Sep 19, 2022, 1:00 AM Jason Cox @.***> wrote:

I'll release this as v1.7.0 for the Arduino library https://www.arduino.cc/reference/en/libraries/tm1637tinydisplay/ and add a TODO to port this to the 6-digit display as well.

Thanks for this great enhancement @hackerceo https://github.com/hackerceo !

— Reply to this email directly, view it on GitHub https://github.com/jasonacox/TM1637TinyDisplay/issues/22#issuecomment-1250571220, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAWCKMT6EFJAAWZEKD7KAEDV67XP5ANCNFSM6AAAAAAQAQE3GI . You are receiving this because you were mentioned.Message ID: @.***>

jasonacox commented 1 year ago

Agree. You added that to library.properties so it now shows up on the Arduino library page:

image

The update announcement was broadcast on twitter -- I added comments: https://twitter.com/jasonacox/status/1572097524223311872?s=20&t=KQt712j4CPpizeAMyt8vng

Thanks again @hackerceo !

hackerceo commented 1 year ago

I agree with your suggestion about the usePROGMEM parameter. I could even agree with adding a second set of functions ending in "_P" but I think it would be cleaner to have a single set of functions that do something rather than 2 sets of functions that do the same thing but are passed data that needs to be processed in a slightly different way.

Do you want to make the changes or should I?

I also noticed that you are using a build process for checking for breaks. The new non-blocking methods are not in the test.ino sketch yet.

Best Regards, Nick Benik

On Sun, Sep 18, 2022, 1:54 AM Jason Cox @.***> wrote:

Thanks for the PR here #24 https://github.com/jasonacox/TM1637TinyDisplay/pull/24 ! I can't wait to use this for my projects.

I have one minor change suggestion for API consistency:

Can we change the parameters for scrollString() and startAnimation() where we put content payload first and put bool usePROGMEM = false at the end with a default to false? This would align with the rest of the function. We could even add scrollString_P() and startAnimation_P() as alias functions that call their main functions with usePROGMEM = true.

Seems like that would help with consistency, but I could be convinced otherwise. What do you think?

void startAnimation(const uint8_t (*data)[4], unsigned int frames = 0, unsigned int ms = 10, bool usePROGMEM = false ); void scrollString(const char s[], unsigned int ms = DEFAULT_SCROLL_DELAY, bool usePROGMEM = false);

— Reply to this email directly, view it on GitHub https://github.com/jasonacox/TM1637TinyDisplay/issues/22#issuecomment-1250198425, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAWCKMUVECH7Y4WO3C4ODZLV62VBBANCNFSM6AAAAAAQAQE3GI . You are receiving this because you were mentioned.Message ID: @.***>

hackerceo commented 1 year ago

I have submitted https://github.com/jasonacox/TM1637TinyDisplay/pull/31 which adds support for non-blocking animations on 6 digit displays. Once PR #31 is merged in you can close this issue.