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

feat: allow full script syntax in event section #112

Merged
merged 2 commits into from
Jun 15, 2024

Conversation

akloeckner
Copy link
Contributor

This upgrades the current "list of services" restriction to allow the full syntax of HA scripts.

fixes #65

As a side-effect this also requires to generate a context for the script to run.

see #96

This can also be seen as a preparatory step to allow defining HASPObject-wide variables.

see #63

I believe, this should be even backwards-compatible, because a list of services is also a valid script.

@dgomes
Copy link
Collaborator

dgomes commented Aug 8, 2023

This is nice... but it moves from services to scripts

Don't think this is the best path... it will cause breaking changes in most setups

@akloeckner
Copy link
Contributor Author

Isn't a "list of services" also a valid "script"? If it is, this should not break anything. Instead, it should make possible a whole range of new things, such as requested in the referenced issues.

But I might miss something not so obvious?

(As a side-effect, we would get in line with most of the other integrations. At least I am not aware of a single integration, that allows only a subset of the script syntax as actions. Again, I might miss something.)

@dgomes
Copy link
Collaborator

dgomes commented Aug 8, 2023

This custom integration was based on https://www.home-assistant.io/integrations/universal/ which supports services only.

How to deal with the breaking change ?

@akloeckner
Copy link
Contributor Author

This custom integration was based on https://www.home-assistant.io/integrations/universal/ which supports services only.

Ah, I see. It even only admits a single service, which is not supported here, as far as I can see. (A list is required.)

How to deal with the breaking change ?

Not sure, if this question goes to me. If it does: I still don't see anything that could break. But I still might be mistaken. Just in case, a notice could be added in the release page as was done for 0.6.0. (If it does not: Never mind my answer. 😉)

@dgomes
Copy link
Collaborator

dgomes commented Aug 9, 2023

@nagyrobi since you have a big setup, can you test this PR :) ?

@akloeckner
Copy link
Contributor Author

Hey, any news on this? I still don't see any breaking change here. Also, I have this running on my instance for half a year now, with no problems so far. The opposite: it allows me to do a whole range of things which wouldn't be possible with services alone. I'll give examples, if you like.

In my opinion: even if this were a breaking change, the opportunities outweigh the risks, by far. But that would be up to you too judge.

For curiosity, I also checked the history, and the actual breaking change was in this commit: e31fd76

Bottom line: I really think this is a useful contribution, which I would rather like to see in the upstream codebase than in my personal fork, which noone uses but me. Especially, since 0.7.0 seems to be approaching (the documentation started to prompt my to switch to it's most current version 0.7.0 lately).

@akloeckner
Copy link
Contributor Author

Checking back... Any news here?

@nagyrobi
Copy link
Collaborator

nagyrobi commented Mar 2, 2024

@nagyrobi since you have a big setup, can you test this PR :) ?

I'm sorry not anymore. I moved to ESPHome with all my displays meanwhile.

@dgomes
Copy link
Collaborator

dgomes commented Mar 3, 2024

!Offtopic

@nagyrobi would love to see a comparison feature wise :)

@nagyrobi
Copy link
Collaborator

nagyrobi commented Mar 3, 2024

@dgomes LVGL is still work in progress but I was able to migrate almost all functionality I previously had.

@dgomes
Copy link
Collaborator

dgomes commented Mar 3, 2024

like the fact that logic can be handled on the device, but coding everything in through yaml... not the best option IMHO. JSONL approach seams more sensible to me.

@nagyrobi
Copy link
Collaborator

nagyrobi commented Mar 3, 2024

I always looked for a way to ditch MQTT from the system, now this is finally possible. Also, for our needs I need zero templates, automations and customizations on HA side, I can do HA service calls directly from the device, no middleware required. HA seems to be faster without the ~200 templates I prevoiusly had to maintain for 6 displays.

I can have any degree of flexibility I want, from C++ lambdas even back to templates.
Conceptually ESPHome is brilliantly thought out. Compilation is very flexible, it generates smaller binaries with only the desired functionality and no data loss.
Encryption is at hands by default and simple to set up.
Only one file to maintain per device.

After migration, family members got back the same design and functionality.

Free heap on the Lanbons previously was 25-30k, now it's aound 100k. This gives room for further expansion, not only additional pages, but also any modern sensor is rather easy to implement.

@nagyrobi
Copy link
Collaborator

nagyrobi commented Mar 3, 2024

Oh and moodlight color is restored across reboots every time regardless of HA or device state as it's being stored on the device itself.

@akloeckner
Copy link
Contributor Author

🤔 Makes me wonder if I should abandon this PR altogether and switch as well. 😉

Jokes aside: Before I find the time for such an adventure, I would still highly appreciate the script feature to become part of this project. And be it only for those who follow...

@dgomes
Copy link
Collaborator

dgomes commented Mar 3, 2024

Oh and moodlight color is restored across reboots every time regardless of HA or device state as it's being stored on the device itself.

This could be easily added to the current firmware...

!Back_to_topic

the breaking change would be changing from single service to list of services... but if we are releasing 0.7 I guess we can accept some breaking changes @fvanroie

@akloeckner
Copy link
Contributor Author

Just to remind: the breaking change from single service to list of services has been done way back in the past. From my I understanding, we're discussing a pure extension of functionality here, without breaking changes.

@dgomes
Copy link
Collaborator

dgomes commented Mar 3, 2024

I'm worried with the EVENT_SCHEMA validation change

@akloeckner
Copy link
Contributor Author

Ah, ok. I'll check more thoroughly:

So, I'm confident that the current [SERVICE_SCHEMA] check is actually a special case of the SCRIPT_SCHEMA check.

Which makes sense, because a list of service calls is a perfectly valid script.

@nagyrobi
Copy link
Collaborator

nagyrobi commented Mar 4, 2024

This could be easily added to the current firmware...

Doesn't seem so, waiting for it for almost 3 years now: HASwitchPlate/openHASP#99 (comment)

@fvanroie
Copy link
Collaborator

fvanroie commented Mar 4, 2024

This could be easily added to the current firmware...

Doesn't seem so, waiting for it for almost 3 years now: HASwitchPlate/openHASP#99 (comment)

That issue is closed now... it doesn't seem anyone missed that feature in the past 3 years.

@nagyrobi
Copy link
Collaborator

nagyrobi commented Mar 4, 2024

That issue is closed now... it doesn't seem anyone missed that feature in the past 3 years.

I closed it 2 weeks ago, after I finally decided that I switch to ESPHome.

@fvanroie
Copy link
Collaborator

Is this still active?

@akloeckner
Copy link
Contributor Author

Yes. It's running on my system since I started the PR (more or less). Without any problems.

This upgrades the current "list of services" restriction to allow the full syntax of HA scripts.

fixes HASwitchPlate#65

As a side-effect this also requires to generate a context for the script to run.

see HASwitchPlate#96

This can also be seen as a preparatory step to allow defining HASPObject-wide variables.

see HASwitchPlate#63
I just read the contributing guidelines...
@akloeckner
Copy link
Contributor Author

I rebased onto main to test #122. No further changes.

@akloeckner
Copy link
Contributor Author

Is this still active?

I was wondering: is there anything else you need from me to proceed on this?

@fvanroie
Copy link
Collaborator

I have no knowledge of the HA inner workings to decide one way or the other. I will let you guys figure it out.
I was just curious since this issue has been open for some time now.

@akloeckner
Copy link
Contributor Author

Ok, thanks for clarifying! To my understanding, Diogo had approved the changes on March 3. So, I assumed this had handed it over to you somehow.

@dgomes, anything missing from me? Please let me know!

@dgomes
Copy link
Collaborator

dgomes commented Jun 15, 2024

I think this is good to go

@fvanroie fvanroie merged commit 5dfa78a into HASwitchPlate:main Jun 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: allow condition in events
4 participants