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

[WIP] Migration of expire binding to profiles #744

Closed
wants to merge 2 commits into from

Conversation

clinique
Copy link
Contributor

@clinique clinique commented Apr 18, 2019

Hello, following discussion initiated in previous PR, here is a first step of work on the Expire transposition to profile.
I'd like your thoughts, and especially regarding the naïve approach I found to transform state/command string to State / Command. Maybe a better way exists, but I found no link with the channel or item.

Also if someone can give me some pointers on the way to proceed with the Registry ... would be welcome :)

Signed-off-by: Gaël L'hopital gael@lhopital.org

@5iver
Copy link

5iver commented Apr 18, 2019

Seems to me like you're addng automation functionality into profiles, and that this might be better as an ActionType. All of the OH1 and Core Actions need to be migrated, and then they can be utilized in the new rule engine, scripted automation, and the rules DSL. There's some discussion and prep work to be done for this, and I plan to get to it soon. Maybe this would interest you?

IMO, an Expire Action sounds better than an Expire Profile.

@kaikreuzer
Copy link
Member

@openhab-5iver I agree that profiles have some overlap with rules. Actually, if you read what I had written about profiles, you will see that I described them to be some implicit rules that the users do not have to create.
In this respect, I don't think an Expire action would be the right way to go - it would mean that users have to create many rules in order to set up this logic.

The one problem that I see with the profile approach is that it cannot be combined with other profiles. If you are e.g. using an Offset profile already, you cannot use the Expire profile at the same time. I am thus not convinced that this is a good approach. It might require a closer integration into the framework, similar to the autoupdate feature.

@5iver
Copy link

5iver commented Apr 20, 2019

I understand the overlap between profiles and automation, but timer functionality also needs to be implemented as an action. Something triggers, conditions are met, and then after a certain amount of time, something else happens (with the option of rescheduling). Having an action and profile doing the same thing will likely cause confusion, especially when a timer action is applied to an expire profile item...

@kaikreuzer
Copy link
Member

I consider the expire feature to be similar to the autoupdate feature: It automatically sends state updates for items. When and why this happens should not matter from the outside and thus it is imho not relevant that scheduled jobs are used internally - they must imho not be exposed, but only be treated as an internal implementation detail.

@davidgraeff
Copy link
Member

davidgraeff commented Apr 24, 2019

I like the simplicity of this solution. It is correct, that if you setup such an expire profile, you loose the ability to assign another profile (or a second expire profile).

they must imho not be exposed, but only be treated as an internal implementation detail.

@kaikreuzer I'm not sure if you have read my last comment in the linked PR. I do agree that implementation details must not be shown, but there must be away to enumerate current setup timers for maintenance reasons.

I have suggested to register those created timers to a Registry and expose them via rest. An alternative is item meta data which can already be requested by REST (if #645 is fixed at least).

@kaikreuzer
Copy link
Member

there must be away to enumerate current setup timers for maintenance reasons.

Could you elaborate on this a bit? I do not see the need for a Registry, which would mean that we open them up to CRUD operations instead of treating them as an internal implementation detail.

@davidgraeff
Copy link
Member

It does not need to be a Registry, this is just what we have.

An overview of setup timers is just useful. Imagine a light turns on/off randomly and you want to find out why. (I'm all for a log-file-free smarthome usage, if that's not yet obvious.) So I would lookup that list of scheduled item changes. Another reason is resource usage. If your smarthome system is unresponsive around every midnight, I'd check my timed automation rules and that item-specific timer list.

openHAB should not hide anything when it comes to scheduled item changes.

@kaikreuzer
Copy link
Member

For rules, I agree that scheduled jobs should be manageable by the user. For anything that is done internally, I think it is dangerous to expose it. With the same logic, you could ask to expose all jobs in any scheduled executor that is used in the code.
For maintenance/performance analysis, this is a classical case for using the metrics API.

@davidgraeff
Copy link
Member

Actually yes I also like the idea of a list of what the scheduler bundle has scheduled. The thing with home-automation software is, you are the user AND the administrator. So it comes in handy to know that your system is scheduling about a ton of work and where that work originates from.

But back to the expire topic: Where do you see harm if an expire2 provides an enumeration interface? (Not CRUD, just read-only)

@kaikreuzer
Copy link
Member

Where do you see harm if an expire2 provides an enumeration interface?

As mentioned above, to me such an information is a metric about the inner state of the software and thus the metric API would be the right thing to use. Yes, I am aware that we do not have an implementation that picks up and display those provided metrics, but it is something we should add - we would have other internal metrics like CPU, Java heap usage, no of threads, threadpool queue lengths, etc. at the same time.

@davidgraeff
Copy link
Member

Didn't know that metrics API was a thing. I was going to submit my solution this weekend. I need exactly that for my paper UI design study.

Signed-off-by: Gaël L'hopital <gael@lhopital.org>
Conflicts:
	bundles/org.openhab.core.thing/src/main/java/org/eclipse/smarthome/core/thing/internal/profiles/SystemProfileFactory.java
	bundles/org.openhab.core.thing/src/main/java/org/eclipse/smarthome/core/thing/profiles/SystemProfiles.java

Signed-off-by: Gaël L'hopital <gael@lhopital.org>
@kaikreuzer
Copy link
Member

Didn't know that metrics API was a thing.

The discussion happened quite a while ago at the ESH forum as well as in the openHAB forum.

We had added the microprofile metrics api to ESH, but didn't yet make any use of it.
But @pokerazor offered to contribute an implementation for pushing the metrics to Prometheus - @pokerazor, maybe you want to comment on this?

@davidgraeff
Copy link
Member

I see. I'll have a look at Eclipse Microprofile Metrics then

@pokerazor
Copy link

@kaikreuzer @davidgraeff yes, we have a working implementation of metrics compatible with Prometheus, as stated in the linked forum post. It is here: https://github.com/KuguHome/openhab-prometheus-metrics
It exposes lots of OH internal metrics (resource usage, scheduler jobs etc) for a detailed insight.
I would be quite happy to discuss how to introduce this into the core.

This is not using the Eclipse Microprofile Metrics though. I have no experience with that. Jochen Hiller of Telekom said they had some implementation using it and were willing to release it when the retraction happend...

This is leading a bit off the original issue topic here, though? :)

@kaikreuzer
Copy link
Member

This is leading a bit off the original issue topic here, though? :)

You're right, I have created #774 as a follow-up!

@wborn wborn added the work in progress A PR that is not yet ready to be merged label May 22, 2019
@Rossko57
Copy link

Rossko57 commented Jun 3, 2019

Comment from non-technical user:

expire-1 is frequently used with "virtual" Items i.e. no bindings (channels). It's not clear to me how you could apply a profile to this.

A frequently noted limitation of expire-1 is that the user cannot change the time delay from rules. But very often, that is a frustration because we're using a virtual item with expire-1 as a simple to manage general purpose timer (for example to block message flooding), and not the original "expire stale data" intent.

A less frequently noted limitation of expire-1 is that the user cannot test for the unexpired time value. Again, that's really about (mis)use of expire for general timing purposes.

@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/is-the-expire-binding-going-away/87395/10

@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/openhab-file-locations/94754/16

@kaikreuzer
Copy link
Member

I'll close this PR as I think it became clear that profiles aren't necessarily the best match for this functionality.
I have created #1620 with an alternative suggestion, which should hopefully also not mean too much effort to implement.

@kaikreuzer kaikreuzer closed this Sep 3, 2020
@kaikreuzer kaikreuzer removed the work in progress A PR that is not yet ready to be merged label Sep 3, 2020
Rosi2143 pushed a commit to Rosi2143/openhab-core that referenced this pull request Dec 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature of the Core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants