[Rules] Improve rules matching (#2660) #2667

TD-er · 2019-10-15T16:42:20Z

Detecting time compare and value compare could give false matches in some cases.
This PR also incorporates a number of other changes:

Adds option to change the remote unit on remote fed plugins via p2p. (Improve usability of ESPEasy p2p protocol #2679)
Cleanup of use of taskIndex, DeviceIndex (Bug in determine index of Plugin_ptr #2676)
Disable and mark configured plugins not included in the build (e.g. when switching builds with other plugins included) (related to Bug in determine index of Plugin_ptr #2676)
Fix build issues with PlatformIO using Python3
Perform first read of ADC at init, before starting WiFi (Analog Input - internal returns 0 first time after Power on / Reboot #2646)

Fixes: #2683 - Duplicated section in platformio.ini
Fixes: #2681 - sendtohttp malformed request
Fixes: #2660 - json update of Domoticz does not work anymore
Fixes: #2676 - Bug in determine index of Plugin_ptr
Fixes: #2670 - Build error P089
Fixes: #2655 - [int#1] does add a leading space in rules
Fixes: #2650 - AP Mode not working when WIFISSID is set
Fixes: #2686 - Argument parsing does not use a single function
Fixes: #2657 - [Rules] Parsing of [task#var] syntax changed since 20190928

Detecting time compare and value compare could give false matches in some cases.

PlatformIO 4.0.x does have issues with non-ASCII characters in files. Even if they are part of comments.

…#2676) See letscontrolit#2676 The use of DeviceIndex was not consistent, which may lead to strange issues. DeviceIndex is the translation between the vector of included plugins and IDs of plugins available.

When using settings containing configuration for a certain plugin which is not included in the actual build, then the plugin must be displayed as "not supported" and not possible to edit. Also show some indication about the consequences of trying to edit such a task. It will then present a dialog equal to adding a new device to a task.

…trolit#2679) See also letscontrolit#2679

Some structs like controller queue elements had their members not sorted to size. This may result in more memory usage than needed.

…#2660) The changed rules parsing may have added some spaces in replaced variables. Adding a trim() call to the parseString does remove any existing or added spaces in the parameter.

The ESP could sometimes experience a crash when saving the rules. This was caused mainly on fragmented file systems and a bit longer rules files. Now the same saveToFile function is used as being used in all other settings file access. Also the rules were copied in memory, which is useless since they are already present in memory (LWIP code) and passed by const reference.

…ntrolit#2646) See letscontrolit#2646

@clumsy-stefan

As noted by letscontrolit#2681 (comment) @clumsy-stefan

clumsy-stefan · 2019-10-25T12:07:52Z

If I understand correctly, Rules still chekc only for the first occurence of a specifig task name and then if the value in the rule is in this task. Therefore it's not possible to have multiple tasks with the same name to be used in rules.

Would'nt it be more logical (and quite easy to solve) if the rules would continue to search for a task with the same name if in the first (previous) occurence no value name that matches is found.

As an example: I would want all possible systemvalues in one common "taskname" (for some controllers this means "the same device") eg. sysinfo but as I'm limited to 4 per task I need multiple tasks for all values. Same for dummy-values. However I can't use them anymore in Rules hen, as only the first occurence is checked..

Just a question/suggestion

TD-er · 2019-10-25T12:18:57Z

A lot of code does make the assumption a task name is unique.
I also added an error message a while ago when you try to save a task with a name that is already present.

The variable names in a task may also be re-used for other tasks.
The true constraint must be that the [taskname#varname] is unique.
Currently we have it a bit more strict to also have the restriction that the task name is unique, since it will become very confusing otherwise.

For example:

task1: foo, variable barone
task1: foo, variable bartwo

It just does not make sense to have multiple tasks with the same name.
If you need it for administrating in controllers, then we may need to add a system variable like %pluginid% or %pluginname% and not make the handling of addressing a specific value more ambiguous.

clumsy-stefan · 2019-10-25T12:24:05Z

The true constraint must be that the [taskname#varname] is unique.

Yep, this is also what I would expect, so it would cover all cases...

It just does not make sense to have multiple tasks with the same name.

hmm.. I think this is POV thing. for me it's totally reasonable... eg. if I need more than 4 dummy vars, why should I identify them via task-name and not via variable name (from hierarchical POV it just makes more sense)...

But again IMHO and I can live with how it's now... as long as it's only a "warning" and no restirction. it's then just not possible to use a variable in a task with duplicat name (in rules)...

TD-er · 2019-10-25T12:31:00Z

Like I said, if you need it for administrative purposes in a controller to set the MQTT topic for example, we may need to have the plugin ID resolved in the controller.

I'm not 100% sure if we can also match an event on only the task name.
If that cannot be done, then it may be an option to have the restriction to have [taskname#varname] unique.
But changing that will not be an easy or quick fix.
For example it would mean there is no longer an 1-to-1 relation anymore between task and task-name and that does have quite a lot of consequences.

clumsy-stefan · 2019-10-25T12:34:14Z

hmm.. but in the rules when you check for [task#value] you only check in the first occurence of task if a coresponding valuein there is not found it just returnes (false). instead of stop checking rules then you could continue cycling through the tasks until TASK_MAX instead of stop checking (so basically jsut finish the loop if the value is not found).

TD-er · 2019-10-25T12:39:10Z

Hmm if that's true, then that is a bug.
An event should match all what is defined in the event.

on taskname#* do should trigger on any event starting with taskname#
on taskname#varname do should trigger only on events matching taskname#varname

Still, the asterisk could give the false impression on which plugin was updated and you will then use a variable from another plugin which was not yet updated.

clumsy-stefan · 2019-10-25T12:41:58Z

I always check for task#value but it only matches if the corresponding value is in first occurence of task.

Eg: if I have 2 tasks witch are called dummy and have value[1-4] in the first and vlaue[5-8] in the second I can never match for dummy#vlaue5.

TD-er · 2019-10-25T12:45:32Z

Ah OK, I get it.
That has to do with the searching for task names indeed.
Not sure about the implications when I would extend the search.
For sure the caching of the search for task names has to be re-written, since you would then require a 1-to-many relation.

Maybe you can add an issue for it so we can continue the discussion there instead of at the comments section of a pull request which will be merged one day :)

…tscontrolit#2650) See letscontrolit#2650. As long as a client is connected to the node in AP mode, no automatic reconnect should be attempted since it will make it very hard to get connected or remain connected to the AP mode. The WiFi channel will change during scan which will disconnect the user.

…rolit#2650) See letscontrolit#2650 The counter wifi_connect_attempt is being used to determine whether the fallback SSID should be tried. It is also used to determine whether a "quick reconnect" is possible using the BSSID and channel of the last successful connection. If the connection is unstable, (last connection was active for over 5 minutes) then the connect attempt counter can be reset.

…olit#2657) See letscontrolit#2657

See: letscontrolit#2686

…rolit#2686)

To be used for the static analysis tools of PlatformIO. See [announcement PIO Home 3.0 beta](https://community.platformio.org/t/platformio-home-3-0-beta-and-powerful-project-inspection/10187?utm_source=platformio&utm_medium=piohome)

TD-er added 10 commits October 15, 2019 18:41

[Rules] Improve rules matching (letscontrolit#2660)

575ec3e

Detecting time compare and value compare could give false matches in some cases.

[PIO] Fix build issue using Python 3

e97d44a

PlatformIO 4.0.x does have issues with non-ASCII characters in files. Even if they are part of comments.

[PIO] Fix build issue due to duplicate env in platformio.ini

d42091f

[Rules] Make handling compare condition more deterministic.

35bb526

[Cleanup] Make use of consistent names for DeviceIndex (letscontrolit…

3c090f1

…#2676) See letscontrolit#2676 The use of DeviceIndex was not consistent, which may lead to strange issues. DeviceIndex is the translation between the vector of included plugins and IDs of plugins available.

[ESPEasy p2p] Allow changing remote unit on receiving plugin (letscon…

1a6262d

…trolit#2679) See also letscontrolit#2679

[Cleanup] Use of taskIndex_t & reduce memory usage in structs

68836da

Some structs like controller queue elements had their members not sorted to size. This may result in more memory usage than needed.

[Cleanup] Use taskIndex_t everywhere where a task index is used.

f8168e9

[sendToHttp] Trim spaces from path (letscontrolit#2681, letscontrolit…

8792e58

…#2660) The changed rules parsing may have added some spaces in replaced variables. Adding a trim() call to the parseString does remove any existing or added spaces in the parameter.

clumsy-stefan mentioned this pull request Oct 25, 2019

sendtohttp malformed request, release mega-20191003 #2681

Closed

TD-er added 2 commits October 25, 2019 12:18

[ADC] Perform first read of ADC at init, before starting WiFi (letsco…

62c422e

…ntrolit#2646) See letscontrolit#2646

TD-er mentioned this pull request Oct 25, 2019

Analog Input - internal returns 0 first time after Power on / Reboot #2646

Closed

[ArduinoIDE] Fix include errors for ArduinoIDE

fdd6e28

As noted by letscontrolit#2681 (comment) @clumsy-stefan

TD-er mentioned this pull request Oct 25, 2019

[Rules] Parsing of [task#var] syntax changed since 20190928 #2657

Closed

TD-er added 2 commits October 25, 2019 17:32

[MQTT] Use parseString functions to disect publish command (letscontr…

709149e

…olit#2657) See letscontrolit#2657

TD-er mentioned this pull request Oct 25, 2019

Home Assistant (openHAB) MQTT controller is exhausting RAM during reconnect #2684

Closed

TD-er added 2 commits October 26, 2019 15:01

[Rules] Fix grammar in parsing parameters (letscontrolit#2686)

0816cd7

See: letscontrolit#2686

[Commands] Mark commands not using standard parse functions (letscont…

62b14d7

…rolit#2686)

TD-er merged commit 68cae55 into letscontrolit:mega Oct 27, 2019

TD-er deleted the bugfix/rules_parsing_issue2660 branch October 27, 2019 09:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Rules] Improve rules matching (#2660) #2667

[Rules] Improve rules matching (#2660) #2667

TD-er commented Oct 15, 2019 •

edited

clumsy-stefan commented Oct 25, 2019

TD-er commented Oct 25, 2019 •

edited

clumsy-stefan commented Oct 25, 2019

TD-er commented Oct 25, 2019

clumsy-stefan commented Oct 25, 2019

TD-er commented Oct 25, 2019

clumsy-stefan commented Oct 25, 2019

TD-er commented Oct 25, 2019

[Rules] Improve rules matching (#2660) #2667

[Rules] Improve rules matching (#2660) #2667

Conversation

TD-er commented Oct 15, 2019 • edited

clumsy-stefan commented Oct 25, 2019

TD-er commented Oct 25, 2019 • edited

clumsy-stefan commented Oct 25, 2019

TD-er commented Oct 25, 2019

clumsy-stefan commented Oct 25, 2019

TD-er commented Oct 25, 2019

clumsy-stefan commented Oct 25, 2019

TD-er commented Oct 25, 2019

TD-er commented Oct 15, 2019 •

edited

TD-er commented Oct 25, 2019 •

edited