aderusha / HASwitchPlate

LCD touchscreen for Home Automation
MIT License
732 stars 128 forks source link

Swap out HA scripts for updating text/font size with a python_script #19

Closed rohankapoorcom closed 4 years ago

rohankapoorcom commented 6 years ago

Hey @aderusha,

I found a large bug with my last implementation of this in #17.

HA doesn't allow scripts to get called in parallel so this approach leads to a lot of dropped messages being sent to the HASP and a mess of warning messages in the logs.

I've refactored to use a python_script (which can be called in parallel) as opposed to a couple of HA scripts.

Before:

- service: script.hasp_plate01_update_message
      data:
        objectId: 'p[2].b[7]'
        text: '"{{states.sensor.weather_current.state|wordwrap(20, wrapstring="\\r")}}"'

After:

- service: python_script.hasp_update_message
    data_template:
      nodename: plate01
      object_id: 'p[2].b[7]'
      text: '{{states.sensor.weather_current.state|wordwrap(20, wrapstring="\\r")}}'

To keep this new python script super generic, I've added a nodename parameter to each of the calls. I've also added an optional parameter update_font which needs to be set to false if the font shouldn't be updated (for example the play/pause button).

I've also updated the README to indicate the new dependency on python_script as well as updated the deployhasp.sh script to get the python script as well.

rohankapoorcom commented 6 years ago

Ping @aderusha - have you had a chance to look at this? I have some more stuff coming that builds on top of this, so wanted to make sure you were good with this before I added more.

aderusha commented 6 years ago

Hey sorry for the delay. I haven't yet had a chance to dig into this. I have some reservations about deploying python scripts in addition to the already-complicated packages just due to it being another level of challenge for new users. Having said that, I do think there may be merit to the approach. I'm a little buried with work and it'll probably be another couple of weeks before I stand up a dev environment to test this and some other suggestions being put forward.

rohankapoorcom commented 6 years ago

Sounds good, let me know once you've taken a look and if you have any questions/concerns. I've been using these python scripts as the basis for the custom component (#24) which should make setup for new users much easier.

rohankapoorcom commented 6 years ago

@aderusha I think you still need to merge this one into python_script

aderusha commented 6 years ago

Hey sorry about that, being kinda github-stupid I've managed to break this. I'm going to continue working on getting this aligned with the current dev branch.

Do you feel like you have the code where you want it? Is it working to your expectations, and do you feel like it's ready for release on your side?

aderusha commented 4 years ago

Looks like this might have been abandoned. Closing PR.

rohankapoorcom commented 4 years ago

Abandoned? Not at all. I use these scripts every day. Just missed you last comment asking for confirmation that they are ready for release.

aderusha commented 4 years ago

Oh hey, sorry man! I'm going to re-open this but at this point in time it's going to need some major work.

First, and I say this with all due respect, I don't think the python component is the way to go here. It's adding complexity while reducing transparency of how things work in the automations, and it appears the one benefit centers around dealing w/ auto-scaling font size based on string length. That particular issue is going to be up-ended a bit as I work on the Nextion 0.55+ firmware which brings with it proportional fonts, so the approach being used in the existing automations and your component is going to have to change as well.

I will however be happy to pull this under the /contrib folder as a standalone, optional feature if folks find it useful.

rohankapoorcom commented 4 years ago

Yeah, no worries, I totally understand.

My personal approach to all software engineering is that you shouldn't repeat yourself and that you use abstraction to simplify what needs to be done repeatedly.

For me that means that the automation doesn't need to be aware that the font size is changing to make things fit. Ideally, that should just happen when the automation changes the text content, which is what the python script allows.

I'm unlikely to upgrade to newer versions of the Nextion firmware anytime soon due to a lack of time as well as the fact that I'm running personally customized versions that are several releases behind.

That said, it might be better to go ahead and close these out for simplicity. They can always be reopened in the future.

aderusha commented 4 years ago

Understood, and thanks for all the work despite the outcome - I really do appreciate it!