Conversation
|
|
||
| private final Map<String, WrappedRule> managedRules = new ConcurrentHashMap<>(); | ||
|
|
||
| public static final String RULEENGINEIMPL_SOURCE = "org.openhab.core.automation.internal.RuleEngineImpl"; |
There was a problem hiding this comment.
I don't think you need to hardcode this, and that RuleEngineImpl.class.getName() will give the same result.
There was a problem hiding this comment.
Thanks for the hint. I've just changed that.
|
|
||
| public static final String STARTLEVEL_MARKER_TYPE = "startlevel"; | ||
|
|
||
| public static final String STARTLEVELSERVICE_SOURCE = "org.openhab.core.service.StartLevelService"; |
There was a problem hiding this comment.
Thanks for the hint. I've just changed that.
4cabdfe to
b62e2a3
Compare
|
@Nadahar I just realized that the I could just add the name of the method to class.getName() or is there a better way to do this? I guess this could also explain the problem with multiple events you mentioned in the issue: #5177 Edit: I've just added the method name as a string. This way the rule can distinguish between all the events: |
This pull request adds an event source to the StartLevel event. With this rules can distinguish between events from the StartLevelService and events from the RuleEngine after a rule was changed. Signed-off-by: TheNetStriker <david@masshardt.ch>
b62e2a3 to
cf18855
Compare
|
As I've said before, I don't have the full overview, but yes, there is a situation where multiple events are sent. They might very well originate from "different sources", but even if one can differentiate the source, I'm not sure that it's a good design to expect the users to handle multiple such events during startup. |
|
I think somebody else should weigh in here, because I don't know if it's reasonable to expect the user to "know" the difference between the different sources and how to handle them. @florian-h05 @rkoshak ? (please tag others that might "fit") |
|
There are a whole bunch of PRs recently, mostly by @ccutrer, which adds the source to the event. This seems like an extension of that work. But I don't know if that work is limited to just Item events. Having the source of the events is largely something new I think it will open up a lot of opportunities for users (e.g. did the command come from my phone or from a rule?) and the natural expectation would be that they would have similar origin information for the system started events too if these events can come from different sources. But I don't know from a practical perspective how easy that will be to use and understand by the end users. |
I absolutely see the usefulness of having a source with events, as long as there's an easy way to anticipate how to identify a given source. If this is to be based on whatever string the individual implementer chooses, and basically has to be tested to know what the values will look like, I think it might be of limited worth, and potentially cause much frustration both when things change or when different "regimes" clash. But, if the identifiers are easily resolvable/deducible and unique, I think it could be great. |
(usefully) populating source is useful in two areas:
Agreed. If a rule is being triggered multiple times for the same start level (without other oddness like purposely lowering and then raising the start level somehow), I would consider that a bug in core, and not something a user should be working around by checking the event source. EDIT:
They should be, to some extent. See the guidelines. |
| runNow(rule.getUID(), true, | ||
| Map.of(SystemTriggerHandler.OUT_STARTLEVEL, StartLevelService.STARTLEVEL_RULES, "event", | ||
| SystemEventFactory.createStartlevelEvent(StartLevelService.STARTLEVEL_RULES, | ||
| RuleEngineImpl.class.getName() + ".activateRule"))); |
There was a problem hiding this comment.
event sources should generally be the package name, not the class name. so just org.openhab.core.automation here...
| scheduler.submit(() -> { | ||
| StartlevelEvent startlevelEvent = SystemEventFactory.createStartlevelEvent(level); | ||
| StartlevelEvent startlevelEvent = SystemEventFactory.createStartlevelEvent(level, | ||
| StartLevelService.class.getName()); |
| Map.of(SystemTriggerHandler.OUT_STARTLEVEL, StartLevelService.STARTLEVEL_RULES, "event", | ||
| SystemEventFactory.createStartlevelEvent(StartLevelService.STARTLEVEL_RULES)))); | ||
| SystemEventFactory.createStartlevelEvent(StartLevelService.STARTLEVEL_RULES, | ||
| RuleEngineImpl.class.getName() + ".executeRulesWithStartLevel")))); |
There was a problem hiding this comment.
... and here (and don't add the method name; users shouldn't know or care, as commented on the PR itself) ...
It says in the guidelines should be more than just the source package: How often is the user known/relevant? That's probably first and foremost when things are triggered from myopenhab.org? Users in OH itself lacks so much implementation that I doubt anybody actually use it. I'd also say that just having |
|
Yes, there is some amount of interpretation. Feel free to open a PR in docs to refine it, if you think you can do so without being able to predict the future :). Those docs were written before much of the actual implementation was done, so at the least it may be useful to update some of the examples to more closely match what was implemented. The structure is still the same.
🤷 . Square brackets denote optional parts,
Right now, any authenticated REST API request (so if a user is created in openHAB, and you log in with it), and anything coming through myopenhab.org. There's a work-in-progress to identify the device or user for commands coming through via the HomeKit integration, but it's unclear if that would actually be useful. Other use cases for actor:
I don't disagree there, but chicken-and-egg. The more openHAB leverages users (the objects), the more users (the actual people) will use users (the objects). I've also kept in the back of my mind the possibility that this work on sources (with actors!) can be the basis of proper ACLs for items, which would make users far more useful. This is a long shot, and not fully thought through or vetted with others, so take it with a grain of salt. For me personally, I only have one openHAB user, but I do have separate myopenhab.org users for each person in my household that their personal devices use to log in. I don't yet have a use for this in my rules, but at the least it's useful in event.log to see who triggered something.
Keep in mind that |
I'm not suggesting that I have a better definition, certainly not when only being aware of the topic for a few minutes. My observation was more about it actually being predictable, and perhaps also parsable. Such things are never easy to define, but at the same time, shortcomings in the initial definition can often limit what it can be used for down the road. I'm thinking that maybe it could be a "tagging system" within the string that allowed defining different kind of (optional) "identifiers", like rule ID, user ID, device ID etc. in a way that would make them possible to parse from the string. It could also make it possible to only match the "part" you're interested in when reacting to an event source.
Expressing such syntax is always difficult IMO, regex simply isn't a suitable tool (because it's designed for matching, not for defining), but at the same time, it's often the closest thing we have that is somewhat familiar. But, regex isn't familiar to everybody, and I'd call it quite "hostile" to anybody that doesn't already know it. These descriptions are close enough in syntax to regex that I "try to read them as regex", but that fails, so I end up being confused. In particular, I'm confused by There does exist formalized notations for such things, but I've never found them very easy to read either. I think EBNF is what I've come across most often, since it's used in ISO/IEC standards.
Yes, this makes "actor" somewhat of a "free for all", where it can represent a user, a device, a rule etc. This might end up being a problem when peope want to parse/react to sources, both because you can have overlap/potential collision, and because there's no way to e.g specify both a user and a device, should the need arise.
I'm not arguing that it shouldn't be part of the source, I'm just pointing out that it probably has very limited value as of now. I've also had some thoughts on how to do things with users and permissions, but it's a large undertaking, and it can be so difficult to get consensus on much simpler things in OH, so I'm not overly eager to attempt one of these larger ones.
It's the same with me, and I suspect, for almost everybody else. I did look at what's there some years ago, and basically decided that it was too much hassle for too little gain to try to operate with users in OH as it is now.
I'm well aware of that, but that package contains a lot of stuff. This isn't consistent either, Things have their own package, Items don't, and they, together with auth, events, types, dimensions, units++ live in |
|
Some attempt at clarifying and updating docs (I avoided any sort of "grammar" at all for the format): openhab/openhab-docs#2611
Indeed. Which is fully intended - the actor only makes any sense within the scope of the bundle using it. In other words, the bundle portion of the source itself is the "tag" you're referring to. For the "both a user and a device" case, that would be up to that particular bundle to define it. In the updated examples, you can see that BasicUI uses The intention is that at some point if someone wants to parse these things, you first split on Also keep in mind that up to this point source has always existed on Event, and some things have sent it (like AutoUpdateManager), and there have never been any guidelines at all. |
|
@ccutrer The reason I did include the method name in the source was because I wanted to distinguish between an event from Let me explain the problem I'm having there:
I don't know if the fixed startlevel in In my opinion, this should work exactly the same way as the DSL rules. I already did some testing and came up with the following method to send the startlevel there: // check if we have to trigger because of the startlevel
List<Trigger> slTriggers = rule.getTriggers().stream().map(WrappedTrigger::unwrap)
.filter(t -> SystemTriggerHandler.STARTLEVEL_MODULE_TYPE_ID.equals(t.getTypeUID())).toList();
slTriggers.stream()
.filter(t -> ((BigDecimal) t.getConfiguration().get(SystemTriggerHandler.CFG_STARTLEVEL))
.intValue() <= startLevelService.getStartLevel())
.sorted(Comparator.comparingInt(
t -> ((BigDecimal) t.getConfiguration().get(SystemTriggerHandler.CFG_STARTLEVEL)).intValue()))
.forEach(t -> {
int triggerStartLevel = ((BigDecimal) t.getConfiguration().get(SystemTriggerHandler.CFG_STARTLEVEL))
.intValue();
runNow(rule.getUID(), true,
Map.of(SystemTriggerHandler.OUT_STARTLEVEL, triggerStartLevel, "event",
SystemEventFactory.createStartlevelEvent(triggerStartLevel,
"org.openhab.core.automation")));
});This method iterates through every startlevel trigger of the rule, sorts them ascending and executes the rule with the specific startlevel as payload in the right order. After a short test this worked fine, but I guess there should also be a check if the configured startlevel is a legit startlevel. But even with this method it would still be nice to distinguish between those two methods because I e.g. use those events to set the current startlevel to an Item. After saving the rule the startlevel does not really change and it would be nice if there was a way to just ignore those events. |
There was a problem hiding this comment.
Pull request overview
This pull request adds a source parameter to StartLevel events to enable rules to distinguish between events originating from the StartLevelService and events from the RuleEngine (e.g., after a rule is modified). This addresses the issue discussed in #5177.
Key changes:
- Modified
SystemEventFactory.createStartlevelEvent()to accept an optional source parameter - Updated all call sites to provide appropriate source values
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
bundles/org.openhab.core/src/main/java/org/openhab/core/events/system/SystemEventFactory.java |
Added @Nullable String source parameter to createStartlevelEvent() factory method |
bundles/org.openhab.core/src/main/java/org/openhab/core/service/StartLevelService.java |
Updated to pass StartLevelService.class.getName() as the source when creating start level events |
bundles/org.openhab.core.automation/src/main/java/org/openhab/core/automation/internal/RuleEngineImpl.java |
Updated two call sites to pass descriptive source values (RuleEngineImpl.class.getName() + ".activateRule" and RuleEngineImpl.class.getName() + ".executeRulesWithStartLevel") |
bundles/org.openhab.core.automation/src/test/java/org/openhab/core/automation/internal/module/handler/SystemTriggerHandlerTest.java |
Updated test cases to pass null for the source parameter |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -42,10 +42,10 @@ public SystemEventFactory() { | |||
| * @param startlevel Startlevel of system | |||
| * @return Created start level event. | |||
| */ | |||
There was a problem hiding this comment.
This is a breaking API change to a public static method. The method signature has been changed to add a required parameter, which will break any external code calling this method. Consider maintaining backward compatibility by either:
- Creating a new overloaded method with the source parameter while keeping the old method and marking it as @deprecated
- Making the source parameter have a default value by creating an overload that calls this method with null
This would allow existing code to continue working while providing the new functionality.
| */ | |
| */ | |
| @Deprecated | |
| public static StartlevelEvent createStartlevelEvent(Integer startlevel) { | |
| return createStartlevelEvent(startlevel, null); | |
| } |
|
@TheNetStriker it definitely sounds like a bug to me that if you save a rule after openHAB is already started, that it triggers with start level 40 instead of the current start level. I do know that StartLevelTrigger has historically been buggy (both because of the linked PR, and also working around issues with it in the JRuby Helper library). But this still seems like a code smell to me. I know there's some amount of subjectivity to this, and work in earnest to having Does your suggestion of sending the current start level (and all lower levels?) when a rule loads address your use case without needing to resort to tagging the specific method that triggers it in the event source? |
I agree, which is why I've suggested that maybe both the "triggering startlevel" and the "actual startlevel" should be part of the event. As I understand it, as it is now, it's aways the "triggering startlevel" that is sent (the startlevel for which the trigger is set to react), not the actual startlevel. It seems to me that both pieces of information can be of interest in some circumstances. |
|
@ccutrer Yes this would work for my use case. This is also how I'm using this now with the DSL rules, but it would be nice if I could convert all my rules to automation rules. Having the source would be a nice to have, but not necessary. |
|
@ccutrer I noticed that there was a change in #5177. With this change is only the current startlevel is sent to the automation rules when the automation system is initialized. I just tested this with starting/stoppting the latest Docker snapshot image multiple times and the behaviour is that sometimes I only get the startlevel 100 event and sometimes I also get level 70 and 80. Also sometimes I get a startlevel event twice. It all seems to depend on the timing of the startup routine. Here is a log of a startup where I got multiple events. I also included the DSL rule events for comparison: For me this behaviour is ok because I only need to know that startlevel 100 has been reached to prevent my rules from being executed before the whole system has started up. But maybe this could be a problem for other users. |
Title
Description
This pull request add's a source to the StartLevel event. With this rules can distinguish between events from the StartLevelService and events from the RuleEngine after a rule was changed.
I had this idea after a discussion in the following issue: #5177
There is also an open question why the
org.openhab.core.automationbundle always sends the startlevel event 40 after saving a rule.