search

lubeda / EspHoMaTriXv2

A simple DIY status display with a 8x32 RGB LED matrix, implemented with esphome.io and Home Assistant.
MIT License
269 stars 25 forks source link

Revert "Revert "2024.1.0: Advanced clock - draw date"" #199

Closed andrewjswan closed 7 months ago

andrewjswan commented 7 months ago

Reverts lubeda/EspHoMaTriXv2#198

Are you sure you haven't forgotten about advanced_clock: true ?

andrewjswan commented 7 months ago

https://github.com/lubeda/EspHoMaTriXv2/pull/198#issuecomment-1888887556 + https://github.com/lubeda/EspHoMaTriXv2/pull/199/files#diff-345f4882a9621cc8a04520b628475a419357f3b98b092260e4749d646698ab16L239-L243

andrewjswan commented 7 months ago

I create test branch from https://github.com/andrewjswan/EspHoMaTriXv2/tree/2024.1.0-Advanced_clock_draw_date , add config from https://github.com/lubeda/EspHoMaTriXv2/pull/197#issuecomment-1884511311 to ulanzi-test.yaml: https://github.com/andrewjswan/EspHoMaTriXv2/blob/2024.1.0-Advanced_clock_draw_date-Test/tests/ulanzi-test.yaml#L229-L243 https://github.com/andrewjswan/EspHoMaTriXv2/blob/2024.1.0-Advanced_clock_draw_date-Test/tests/ulanzi-test.yaml#L272-L282

Commit and build complete - https://github.com/andrewjswan/EspHoMaTriXv2/actions/runs/7501846860/job/20423315435

andrewjswan commented 7 months ago

But when I try to make a PR with such a configuration file, I get an error. https://github.com/lubeda/EspHoMaTriXv2/actions/runs/7501946835/job/20423611396 [date_format_big] is an invalid option for [ehmtxv2]. Did you mean [time_format_big], [date_format], [time_format]? Although this is specified in the Python. https://github.com/lubeda/EspHoMaTriXv2/pull/199/files#diff-b95092b415899adcf850b1e4115261c02d0ae5a352deedf4801b5ca740b1f6cb Maybe the problem is in Github Action?

andrewjswan commented 7 months ago

I did an experiment in my repository, made a similar PR (Revert, Revert) accepted it in 2024.1.0, made a PR with a test change in the ulanzi-test.yaml configuration, https://github.com/andrewjswan/EspHoMaTriXv2/commits/2024.1.0-prerelease/ https://github.com/andrewjswan/EspHoMaTriXv2/commit/4c1fc95bb73025611e2739cdbdf8fc4b54e16c39 https://github.com/andrewjswan/EspHoMaTriXv2/blob/4c1fc95bb73025611e2739cdbdf8fc4b54e16c39/tests/ulanzi-test.yaml#L272-L282 and everything build without errors. https://github.com/andrewjswan/EspHoMaTriXv2/actions/runs/7503152730/job/20427316577

andrewjswan commented 7 months ago

If we want to make advanced_clock be the default and always work, then we can enable it by replacing the default value with true https://github.com/lubeda/EspHoMaTriXv2/blob/2024.1.0-prerelease/components/ehmtxv2/__init__.py#L168-L170

lubeda commented 7 months ago

Is true as default value a breaking change? I mean, do I have to use set_clock_infotext_color or set_date_infotext_color or does the "normal" clock work as before?

If it's not breaking, fine, and perhaps this could lead to a little code clean up. e.g. set_clock_color internally calls set_clock_infotext_color and set_date_infotext_color. So only advanced users need to care and normal users (like me) have to change nothing.

Sorry, I'm low on spare time.

andrewjswan commented 7 months ago

Is true as default value a breaking change?

No, everything should work as before without problems.

I mean, do I have to use set_clock_infotext_color or set_date_infotext_color or does the "normal" clock work as before?

When calling set_infotext_color, the color will be set to the same color for set_clock_infotext_color and set_date_infotext_color i.e. you can still have one method setting the color on all screens or do it individually https://github.com/andrewjswan/EspHoMaTriXv2/blob/2024.1.0-prerelease/components/ehmtxv2/EHMTX.cpp#L887-L898

So only advanced users need to care and normal users (like me) have to change nothing.

When this mode is enabled, everything should continue to work as before, i.e. There should be no changes in the rendering or filling of regular methods.

But we can leave this mode disabled by default (as it is now). And then any user will be able to read the documentation, enable it and customize it to their taste.

andrewjswan commented 7 months ago

Sorry, I'm low on spare time.

It's okay, I use this mode every day, so far there have not been a single error or incorrect operation. And we are in no hurry, unless of course there are plans to release the 2024.1.0 release right now. But in my opinion, in its current state the firmware is quite stable and functional.