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

Fix system started DSL rules #3725

Merged
merged 6 commits into from
Jul 23, 2023
Merged

Fix system started DSL rules #3725

merged 6 commits into from
Jul 23, 2023

Conversation

J-N-K
Copy link
Member

@J-N-K J-N-K commented Jul 22, 2023

Fixes #3723

@lolodomo can you test if that resolves your issues? On my system it seems to work fine.

@J-N-K J-N-K added the bug An unexpected problem or unintended behavior of the Core label Jul 22, 2023
@J-N-K J-N-K requested a review from a team as a code owner July 22, 2023 18:20
Signed-off-by: Jan N. Klug <[email protected]>
Signed-off-by: Jan N. Klug <[email protected]>
Signed-off-by: Jan N. Klug <[email protected]>
@J-N-K
Copy link
Member Author

J-N-K commented Jul 22, 2023

I don't have a good setup to test this (and no time), since I neither use file-based rules nor scripts. It should be tested that

  • rules/scripts can be added
  • rules/scripts can be modified, only show up once and execute the new code
  • rules/scripts can be removed

@J-N-K J-N-K requested a review from lolodomo July 22, 2023 18:38
@lolodomo
Copy link
Contributor

After a bundle update and a OH server restart; the warning has disappeared but now the rule is run twice, apparently the second just after the first:

20:39:59.891 [INFO ] [org.openhab.core.model.script.Rule   ] - Démarrage début
20:39:59.907 [INFO ] [org.openhab.core.model.script.Rule   ] - Démarrage fin
20:40:00.002 [INFO ] [org.openhab.core.model.script.Rule   ] - Démarrage début
20:40:00.034 [INFO ] [org.openhab.core.model.script.Rule   ] - Démarrage fin
20:40:00.064 [INFO ] [re.automation.internal.RuleEngineImpl] - Rule engine started.

Signed-off-by: Jan N. Klug <[email protected]>
@J-N-K
Copy link
Member Author

J-N-K commented Jul 22, 2023

I have no idea why that happens, maybe a check for the start-level is missing. Can you post a file that shows this behavior? In my tests the rues run exactly once. Unfortunately I can't work on that today.

@kaikreuzer
Copy link
Member

Can you post a file that shows this behavior?

This file will actually do (adding some logging to it).

I get this result at startup:

23:51:13.363 [INFO ] [org.openhab.core.Activator          ] - Starting openHAB 4.0.0 (build Unknown Build No.)
23:51:16.033 [INFO ] [el.core.internal.ModelRepositoryImpl] - Loading model 'demo.items'
23:51:16.368 [INFO ] [el.core.internal.ModelRepositoryImpl] - Loading model 'mapdb.persist'
23:51:16.549 [INFO ] [el.core.internal.ModelRepositoryImpl] - Loading model 'demo.sitemap'
23:51:16.660 [INFO ] [el.core.internal.ModelRepositoryImpl] - Loading model 'demo.things'
23:51:18.436 [INFO ] [el.core.internal.ModelRepositoryImpl] - Loading model 'demo.rules'
23:51:23.795 [INFO ] [org.openhab.core.model.script.TEST  ] - Rule executed.
23:51:28.769 [INFO ] [org.openhab.core.model.script.TEST  ] - Rule executed.
23:51:28.772 [INFO ] [e.automation.internal.RuleEngineImpl] - Rule engine started.
23:51:28.773 [INFO ] [openhab.event.ItemStateChangedEvent ] - Item 'DemoDateTime' changed from 2023-07-22T23:51:23.799362+0200 to 2023-07-22T23:51:28.771569+0200

So indeed the rule is executed twice and the first time happens quite a time before the rule engine is officially started.

@lolodomo
Copy link
Contributor

I tried again with the last change in the PR but same result, that is rule run twice.

Additionally, fix nullable error and warning in Eclipse IDE

Signed-off-by: Kai Kreuzer <[email protected]>
return s.lines().map(line -> (line.startsWith(indentation) ? line.substring(indentation.length()) : line))
.collect(Collectors.joining("\n"));
}

private @Nullable Trigger mapTrigger(EventTrigger t) {
if (t instanceof SystemOnStartupTrigger) {
Configuration cfg = new Configuration();
cfg.put("startlevel", 20);
cfg.put("startlevel", 40);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change fixes the issue for me. I have no idea, why I have ever set it to 20 in the first place.
According to the documentation at https://www.openhab.org/docs/configuration/rules-dsl.html#system-based-triggers, "startlevels less than 40 are not available as triggers". Setting it to 40 will only execute them once during startup.

@kaikreuzer
Copy link
Member

@lolodomo I just pushed a commit that fixes the issue for me. Would you be able to test this as well, please?

@lolodomo
Copy link
Contributor

@lolodomo I just pushed a commit that fixes the issue for me. Would you be able to test this as well, please?

I do it now...

@kaikreuzer
Copy link
Member

Thanks for your nightshift.

@lolodomo
Copy link
Contributor

Kai, it looks like the rule is run twice but now the second time occurs very late at startup when everything is loaded.

00:46:13.976 [INFO ] [org.openhab.core.model.script.Rule   ] - Démarrage début
00:46:14.005 [INFO ] [org.openhab.core.model.script.Rule   ] - Démarrage fin
00:46:14.036 [INFO ] [re.automation.internal.RuleEngineImpl] - Rule engine started.

...

00:47:34.478 [INFO ] [org.openhab.core.model.script.Rule   ] - Démarrage début
00:47:34.489 [INFO ] [org.openhab.core.model.script.Rule   ] - Démarrage fin

@kaikreuzer
Copy link
Member

I assume your rule has a simple "System started" trigger and nothing else?

@lolodomo
Copy link
Contributor

But I believe it was already happening in the past that this rule was run a second time very late at startup.
Maybe we are back to the previous behaviour.

Signed-off-by: Kai Kreuzer <[email protected]>
@lolodomo
Copy link
Contributor

I assume your rule has a simple "System started" trigger and nothing else?

Yes

rule "Démarrage"
when
	System started
then
	logInfo("Rule","Démarrage début")
	lastDesarmTime = now.minusDays(1)
	lastMinimoteTime = now
	patternVoice = Pattern::compile("(donner|donne|passer en hors gel|passer en mode confort|passer en mode réduit) (le |la |les |l'|du )(.*) (du |de la |de l'|dans le |dans la |dans l')(.*)")
	logInfo("Rule","Démarrage fin")
end

@lolodomo
Copy link
Contributor

I can try quickly with snapshot 3544 to confirm that the rule was already run a second time very late.

@kaikreuzer
Copy link
Member

But I believe it was already happening in the past

Which is still very weird - I never had startup rules being triggered twice.
Is there maybe some mechanism on your system that touches these rules, so that they are reloaded for some reason?

@kaikreuzer
Copy link
Member

I can try quickly with snapshot 3544 to confirm that the rule was already run a second time very late.

Yeah, would be great.

@lolodomo
Copy link
Contributor

I can confirm that the behaviour with snapshot 3544 is the same as the current with this PR.
So there is probably an issue because this rule is run a second time very late but it is not new.

If your tests are positive, I believe we can go with that.

Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thanks for testing - then this at least is nothing newly introduced by this PR.
I hereby approved the changes done by @J-N-K and leave it to him to merge the PR if he approves my changes as well.

@J-N-K J-N-K added this to the 4.0 milestone Jul 23, 2023
Copy link
Member Author

@J-N-K J-N-K left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@J-N-K J-N-K merged commit 546e342 into openhab:main Jul 23, 2023
2 checks passed
@J-N-K J-N-K deleted the fix-dsl branch July 23, 2023 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An unexpected problem or unintended behavior of the Core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New warning "No pre-parsed script found for xxx"
3 participants