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

Triggers are not reloaded on item change #11

Closed
janwo opened this issue Mar 30, 2021 · 16 comments
Closed

Triggers are not reloaded on item change #11

janwo opened this issue Mar 30, 2021 · 16 comments

Comments

@janwo
Copy link

janwo commented Mar 30, 2021

Describe the bug
A clear and concise description of what the bug is.

To Reproduce
Steps to reproduce the behavior:

  1. Add a rule like@when("Member of gMyGroup received update").
  2. Change the members of gMyGroup.
  3. Inspect the triggers via UI.
  4. Added or removed members of the group are not added or removed from the rule triggers.

Expected behavior
All members of the group are part of the rule triggers.

Screenshots
If applicable, add screenshots to help explain your problem.

Environment (please complete the following information):

  • openHAB version: openHAB 3.1.0 Build #2294
  • Release: master
@janwo janwo added the bug Something isn't working label Mar 30, 2021
@CrazyIvan359
Copy link
Owner

This is not a bug, but a limitation of OH. These libraries simulate the Member of trigger by creating a separate item trigger for each member of the group.

I know there is now an actual Member of trigger in OH3, but I don't know if it updates when the group members are changed. If it is dynamic like that then the libraries could be updated to use the native Member of trigger when on OH3.

@janwo
Copy link
Author

janwo commented Mar 30, 2021

Thanks, that sounds reasonable.👍 Is there a workaround?

Enabling and disabling rules did not work. Only re-save them triggers a reinitialization.
Maybe this can be done programmatically?

@CrazyIvan359
Copy link
Owner

The resave will be the easiest way.

I have some dynamically generated rules who's generating code watches for group membership changes and then deletes and recreates the rule. This approach would not be ideal unless turned into a library (which I kinda like the idea of, unless the OH3 member of trigger works with membership changes).

What is your use case for this? Maybe there is another way to handle the situation.

@janwo
Copy link
Author

janwo commented Mar 31, 2021

I like to track the last activation (e.g. Light) of an item with a given tag. Therefore a rule checks upon item changes, if every item of that tag is part of a given group, e.g. gGroupWithAllLightbulbItems. Why this? There is no trigger upon tags.

When I am changing tags, the rule adds or removes items from the group. The other rule that triggers upon group updates, e.g. on Member of gGroupWithAllLightbulbItems, still has the old group memberships in place. Therefore I need to recreate the rule at this time. That's my pain point atm. You have an idea?

Actually I would really appreciate the reloading of rules with Member of or Descendant of triggers unless the corresponding group memberships change. Makes sense imo.

@CrazyIvan359
Copy link
Owner

That sounds like the hard way, what are you triggering with these tags? Wouldn't a DateTimeItem for each light that gets set when it changes be easier?

I agree that dynamic triggers would be better, but it is not a simple thing to do. My current implementation that I use for some of my own rules is not working perfectly even.

@janwo
Copy link
Author

janwo commented Mar 31, 2021

Actually I want to setup everything easily without manual creation of items (so people without detailed knowledge can setup their home easily. So thats why I make much use of the metadata.

The rule to sync the tags with the group:

@when("Item added")
@when("Item updated")
@when("Item removed")
@when("System started")
def sync_lights_helpers(event):
    sync_group_with_tags(
        ir.getItem("gCore_Lights_Switchables"),
        LIGHTS_EQUIPMENT_TAGS # e.g. Lightbulb
    )

The rule to set the last light activation of a location:

@when("Descendent of gCore_Lights_Switchables received update")
def set_last_light_activation(event):
    item = ir.getItem(event.itemName)
    if item.getStateAs(OnOffType) == ON:
        location = get_location(item)
        if location:
            set_key_value(
                location.name,
                METADATA_NAMESPACE,
                'lights',
                "last-activation",
                get_date_string(ZonedDateTime.now())
            )

@CrazyIvan359 Maybe we can improve your logic together? ;-)

@CrazyIvan359
Copy link
Owner

That is the same reason I'm doing mine the way I did, though I'm the only one using it right now. One problem with it is that all items get removed when OH shuts down, which may cause strange behaviour. The bigger issue is bulk updates causing multiple triggers, this can cause strange behaviour if the rule delete/generate code tries to create a trigger for item 2 that was removed while it was running when triggered by item 1 being removed. A single bad trigger will cause the rule not to be created. You could have the regen code pop bad triggers before creating the rule, but doing that blindly could hide other issues (though I suppose you would see the error from when in the log).

I will post my code here later for you to look at. I am working on other projects at the moment so I won't be able to actively test anything, but I will respond to messages.

@CrazyIvan359 CrazyIvan359 removed the bug Something isn't working label Apr 1, 2021
@CrazyIvan359
Copy link
Owner

What you see below is take 2 of this code. Take 1 was ad-hoc and needs improvement but has seen limited production use, the code below is refined and abstracted, though has not seen production use yet.

This is the starting point, this rule triggers on items being added, removed, or changed. There are no triggers here because this code is built to be published this rule is generated once on load. The triggers are "Item added", "Item updated", and "Item removed". The ItemEventxxx classes are my own, I have included that code at the end of this post.

import typing as t

from community.events.items import ItemEventGroupMember

if t.TYPE_CHECKING: # this will only work if using my stubs with typing installed
    from org.openhab.core.items.events import (
        ItemAddedEvent,
        ItemCommandEvent,
        ItemRemovedEvent,
    )

def rule_item_event(event):
    # type: (t.Union[ItemAddedEvent, ItemRemovedEvent, ItemUpdatedEvent]) -> None
    item_event = ItemEventGroupMember(event, GROUP)
    if (
        item_event.added or (item_event.updated and item_event.membership_changed)
    ) and item_event.is_member:
        # item added to GROUP
        # regenerate rule(s)
    elif (item_event.removed and item_event.is_member) or (
        item_event.updated and item_event.membership_changed and item_event.was_member
    ):
        # item removed from GROUP
        # regenerate rules

And here is my rule (re)generator. The lambda bit is hard to explain but it just provides a group name to substitute into each trigger in the list. You may not need anything that fancy if you are using this in one place. RULE_NAME, RULE_DESC, and RULE_TRIG are all dictionaries with function names as keys, the generator looks up using the function passed. These are in the same file as the generator, the generator itself is generic but I have not managed to get it into a separate library yet.

import typing as t

from core.jsr223.scope import scriptExtension
from core.rules import rule
from core.triggers import when
ruleRegistry = scriptExtension.get("ruleRegistry")  # type: RuleRegistry

if t.TYPE_CHECKING:
    from org.openhab.core.automation import RuleRegistry

def generate_rule(target):
    # type: (function) -> None
    """ Creates a rule, removing it if it already exists """
    UID = getattr(target, "UID", None)  # type: str
    if UID:
        ruleRegistry.remove(UID)
        del UID, target.UID
        del target.triggers

    # this is using a map and lambdas to get the group name from the config
    for trigger in RULE_TRIG[target.__name__]:
        when(trigger.format(group=RULE_TRIG_MAP.get(target.__name__, lambda: "")()))(
            target
        )

    triggers = getattr(target, "triggers", None)  # type: t.Union[t.List, None]
    if triggers and triggers.count(None) != 0:
        log.warn("Rule '%s' not created, a trigger is invalid", target.__name__)
    elif triggers:
        rule(RULE_NAME[target.__name__], RULE_DESC[target.__name__], TAGS)(target)
        if getattr(target, "UID", None):
            log.debug("Rule '%s' created successfully", target.__name__)
        else:
            log.error("Failed to create rule '%s'", target.__name__)
    else:
        log.debug("Rule '%s' not created, no valid triggers", target.__name__)
Event Processors

This library pulls out the common code that checks if an item was added, removed, or changed (ItemEventProcessor) and also contains ItemEventGroupMember and ItemEventGroupDescendant which check if an item was added or removed from a group. These classes have not seen much use yet, so you may find errors and I have not documented them yet. You seem pretty competant with Python so I think you'll be able to figure them out.

I have the code below in automation/lib/python/community/events/items.py and you will also need a blank automation/lib/python/community/events/__init__.py for it to work.

__all__ = ["ItemEventProcessor", "ItemEventGroupMember", "ItemEventGroupDescendant"]

import typing as t

from personal.utils import is_member_of_group

from java.lang import String

from org.openhab.core.items import GenericItem
from org.openhab.core.items.dto import ItemDTO
from org.openhab.core.items.events import (
    ItemAddedEvent,
    ItemRemovedEvent,
    ItemUpdatedEvent,
)


class ItemEventProcessor(object):
    __slots__ = ["_added", "_removed", "_updated", "_item", "_old_item"]

    def __init__(self, event):
        # type: (t.Union[ItemAddedEvent, ItemRemovedEvent, ItemUpdatedEvent]) -> None
        self._added = False
        self._removed = False
        self._updated = False
        self._item = None
        self._old_item = None
        if isinstance(event, ItemAddedEvent):
            self._added = True
            self._item = event.getItem()
        elif isinstance(event, ItemRemovedEvent):
            self._removed = True
            self._item = event.getItem()
        elif isinstance(event, ItemUpdatedEvent):
            self._updated = True
            self._item = event.getItem()
            self._old_item = event.getOldItem()

    @property
    def added(self):
        # type: () -> bool
        return self._added

    @property
    def removed(self):
        # type: () -> bool
        return self._removed

    @property
    def updated(self):
        # type: () -> bool
        return self._updated

    @property
    def item(self):
        # type: () -> ItemDTO
        return self._item

    @property
    def old_item(self):
        # type: () -> ItemDTO
        return self._old_item


class ItemEventGroupMember(ItemEventProcessor):
    __slots__ = ["_is_member", "_was_member"]
    _recurse = False

    def __init__(
        self,
        event,  # type: t.Union[ItemAddedEvent, ItemRemovedEvent, ItemUpdatedEvent]
        group,  # type: t.Union[str, String, GenericItem, ItemDTO]
    ):  # type: (...) -> None
        super(ItemEventGroupMember, self).__init__(event)
        self._is_member = False
        self._was_member = False
        if self._item:
            self._is_member = is_member_of_group(self._item, group, self._recurse)
        if self._old_item:
            self._was_member = is_member_of_group(self._old_item, group, self._recurse)
        else:
            self._was_member = self._is_member

    @property
    def is_member(self):
        # type: () -> bool
        return self._is_member

    @property
    def was_member(self):
        # type: () -> bool
        return self._was_member

    @property
    def membership_changed(self):
        # type: () -> bool
        return self._is_member | self._was_member and not (
            self._is_member and self._was_member
        )


class ItemEventGroupDescendant(ItemEventGroupMember):
    _recurse = True

@janwo
Copy link
Author

janwo commented Apr 8, 2021

Looks good, thanks!

I have some questions:

  1. Is the regenerated rule removed after reloading the original python rule?
  2. The Rule UUID of a python rule is always different imo, does the matching work for you?
  3. May you like to share the full code within the community contributions? This way I can test and contribute to it more easily.

When I look among the openHAB community topics, I see this issue multiple times.

@CrazyIvan359
Copy link
Owner

CrazyIvan359 commented Apr 8, 2021

  1. This block deletes the existing rule if there was one:
    ​UID​ ​=​ ​getattr​(​target​, ​"UID"​, ​None​)  ​# type: str​
    ​if​ ​UID​:
        ​ruleRegistry​.​remove​(​UID​)
        ​del​ ​UID​, ​target​.​UID​
        ​del​ ​target​.​triggers
  1. Yes, UUID matching works. The UUID is generated by the rule engine of OH and does not change while the rule exists.

  2. Yes, I plan on sharing this eventually. There is not documentation written about how to use it or what it's for, so that will have to come first.

@janwo
Copy link
Author

janwo commented Apr 13, 2021

I am still experimenting with these functions. How do you create the original triggers here?

for trigger in RULE_TRIG[target.__name__]:
        when(trigger.format(group=RULE_TRIG_MAP.get(target.__name__, lambda: "")()))(
            target
        )

Unfortunately, I always get the resolved (old) triggers of the old rule instance, so there is no Member of >Group< received update triggers in place, although defined in the files. Instead I have a list of triggers with the corresponding group members only, e.g. Item >GroupMember< received update.

@janwo
Copy link
Author

janwo commented Apr 13, 2021

Not easy, surfing through the community forums 🤔 I guess this library needs to save the file metadata in addition to the rule registry (at least the original triggers). This could be done while parsing the rule files. After that we can trigger a reinitialization if those triggers.

Second approach is to get "descendant of" trigger into core" and replace it in triggers.py, see openhab-scripters#318

Third approach is to initialize the file parser to restart the parsing of a whole rule file?

@janwo
Copy link
Author

janwo commented Apr 15, 2021

I added arrays of original triggers of each rule and add this to the following method call:


def reload_rules(tags=['core-reload'], triggers=[]):
    if len(triggers):
        for _rule in ruleRegistry.getByTags(tags):
            ruleRegistry.remove(_rule.getUID())
            for trigger in triggers:
                Log.logError(
                    'reload_rules',
                    'Add trigger {} to rule {}'.format(trigger, _rule.getName())
                )
                when(trigger)(_rule)
            rule(
                _rule.getName(),
                _rule.getDescription(),
                _rule.getTags()
            )(_rule)
    else:
        Log.logError(
            'reload_rules',
            'Could not reload rule with tags {}'.format(tags)
        )

Unfortunately, I receive the following error:
File "/openhab/conf/automation/lib/python/core/triggers.py", line 164, in item_trigger 15.04.21 12:21:18 (+0200) openhab function.triggers.append(ItemStateUpdateTrigger(member.name, state=new_state, trigger_name=trigger_name).trigger) 15.04.21 12:21:18 (+0200) openhab UnsupportedOperationException: java.lang.UnsupportedOperationException

@CrazyIvan359 How can I add those triggers? I would say that I am doing the same as you do.

@CrazyIvan359
Copy link
Owner

RULE_TRIG and the like are dictionaries indexed by function name. The one for triggers has an array of trigger strings that need an item name injected into them, as done by the format call. Ignore the lambda in that format call for now, that is a trick I use to get the item name from configuration.py at runtime.

You are correct that the Member of trigger is always resolved to a trigger for each member item. The OH version of that trigger has not been implemented in the libraries yet, though I believe someone has already opened an issue. It is not required for this code though.

In your last post, I'm not sure the exact source of the error, but it is stemming from you passing a Trigger instance to a function that expects a string. The triggers property contains an array of Trigger objects that get used in the creation of the rule in the rule call. In theory you could try reusing them, but invariably you will need to add or remove one trigger each time the regen rule runs. That is far more complicated than ditching the whole thing and starting over, we aren't running on resource strained embedded systems, we don't need that kind of optimization.

@CrazyIvan359
Copy link
Owner

CrazyIvan359 commented Apr 15, 2021

Maybe this will help, here is a real example of those dicts from one of my other libraries. You don't need the lambda if you aren't pulling the group name from the config.

RULE_TRIG = {
    "rule_link_changed": ["Member of {group} changed"],
}
RULE_TRIG_MAP = {
    "rule_link_changed": lambda: config.GROUPS[KEY_LINKS],
}  # type: t.Dict[str, t.Callable[[], str]]

Without the lambda the loop would look like this:

for trigger in RULE_TRIG[target.__name__]:
        when(trigger.format(group="MY_GROUP_NAME"))(target)

Which would resolve to "Member of MY_GROUP_NAME changed" for the function rule_link_changed.

@janwo
Copy link
Author

janwo commented Apr 18, 2021

Ok, my approach will not solve the whole thing, but I was able to trigger a rule reload. I am using the following method and call it manually as soon as I need a refresh of group members:

# triggers: array of triggers, e.g. ["Member of GROUP received update"]; target: rule function
def reload_rules(triggers, target):
    if not len(triggers) or not target:
        Log.logError(
            'reload_rules',
            'Could not reload rule without triggers or target!'
        )
        return

    UID = getattr(target, "UID", None)
    if not UID:
        Log.logError(
            'reload_rules',
            'Could not reload rule without UID in target object!'
        )
        return

    r = ruleRegistry.get(UID)
    if not r:
        Log.logError(
            'reload_rules',
            'Could not reload rule - uid {} was not found!'.format(UID)
        )
        return

    del target.triggers
    for t in triggers:
        when(t)(target)

    ruleRegistry.remove(UID)
    del UID, target.UID
    rule(
        r.getName(),
        r.getDescription(),
        r.getTags()
    )(target)

Additionally, I also refer to an alternative solution, see here: https://community.openhab.org/t/design-pattern-rule-refresh/102317

Shall we keep this issue open?

@janwo janwo closed this as completed Oct 12, 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

No branches or pull requests

2 participants