Rule Parsing White Space Issue. {Rule Example in Wiki no longer works}

[thomastech]

Background info:
Recently the ESPEasy Rule parameter parsing was refactored. The new requirements were nicely summarized by TD-er in this post:
#2724

The Problem:
But the new parsing has also affected the syntax of conditional statements. Older releases were more tolerant to the whitespace usage in a "if" statetment. For example, the Wiki rule example published here no longer works:
https://www.letscontrolit.com/wiki/index.php/Tutorial_Rules

Original example code:

on Rules#Timer=1 do
  if[E1SW1#Switch]=1
  gpio,12,0
else
  gpio,12,1
endif
timerSet,1,10
endon

This example worked on older versions. But with the new rule parsing this rule will cause abnormal operation, including reboot loops.

The problem is due to this statement:
if[E1SW1#Switch]=1

The new rule parsing requires a space between the if and the [ open bracket. Like this:
if [E1SW1#Switch]=1

Proposed solution:
Revise parsing so that the "if" does not require any whitespace before the "[" open bracket. That is to say, make it backwards compatible.

• Thomas

[TD-er]

That's a good one.

[uzi18]

personally it should be more clear with space, but if "compatibility" mode is used in advenced options it can be tolerated, all possible examples should be with whitespace added or maybe easier is to add syntax parser in JS and warn user about possible problems

[thomastech]

The wiki rule example has been around a long time. So I suspect there will be some existing users that will suffer abnormal rule behavior and/or reboot loops after they flash to the current release.

To ensure that ESPEasy is "easy" to use, I highly recommend that the missing white space is tolerated without further attention by the user.

EDIT: The rules example (jpg image) in the wiki has been updated. I exaggerated the whitespace between the "if" and "[" open bracket. Ref: Rules1.jpg

• Thomas

[TD-er]

Well @uzi18 's suggestion is to only allow it with the "Tolerant last parameter" checked.
And indeed docs should be up-to-date.

[uzi18]

@TD-er this should be easy to find when rules file is loaded into webbrowser - regex like "newline/whitespace+if[" and when it is there warn user about it.

[uzi18]

in fact we can think about it when new issues like this arrive, for now it is not a big problem

[thomastech]

So far I've helped out a couple forum users that have experienced this issue when using the latest releases. For example:
https://www.letscontrolit.com/forum/viewtopic.php?f=6&t=7586&p=43645&hilit=bracket#p43645

And indeed docs should be up-to-date.

For the record, they weren't. So that's why I updated the wiki today.

I hate to sound like a brat, but I don't understand the reluctance to allow backwards compatibility and ignore the missing white pace. There's no need to warn about it, just allow it.

ESPEasy should be easy. Making a change to functionality that breaks existing rules seems like an invitation to user frustration. I'm just here trying to make a point that the user experience is important.

• Thomas

[TD-er]

Well you don't sound like a brat :)

One reason (for me) to be a bit reluctant about backwards compatibility is that it may make the code more elaborate than strictly needed.
And elaborate code has several drawbacks:

• Easy to run into corner cases where new functionality may be next to impossible to implement
• Larger code base
• Less clear code which may result in bugs.

So by using a "backwards compatibility" or "tolerant" mode, you may make it clear some of the currently used code is "deprecated" or at least not as it was intended to be used.

What we can do, to make it "easy" again, is to parse for a few common "mistakes" and warn the user about it.
Such a warning can be a link to a page explaining what may be wrong here and/or how it can be mitigated.
Does that sound like something that would make it a good compromise between "easy to use" and "easy to maintain" ?

[thomastech]

In regards to the warning idea, I can imagine how that could work when the user is saving edits to a Rules page. But it's not clear to me how this helps when the user has re-flashed to a newer version and the white space is missing in their existing if statements. This scenario might suck the "easy" out of ESPEasy (rules stop working and/or possible reboot loop).

Enough of my rants, I don't want to wear out my welcome. So I'll leave the fix implementation up to you.

• Thomas

[TD-er]

This scenario might suck the "easy" out of ESPEasy (rules stop working and/or possible reboot loop).

That's a valid point.

Right now we have a check on reboots by disabling plugins and then controllers if they might be the cause of reboots.
I think a valid test in this cycle can be to enable the "tolerant" option and also present a warning if that has been enabled. (also for other disabled plugins as some users post issues about it)

So I will also append the check for "enable tolerant mode" and next step to disable rules.
Checks for missing space and future breaking changes can also be done at boot, so the user may be warned as soon as possible if there might be something wrong with their settings.
Such a "pre-flight-check" may also be done on often made mistakes (e.g. referring a variable that doesn't exist).

I'm all in favor of making it as easy as possible, but on the other hand remaining compatible with the past may later become a burden.
So somewhere we have to find a good compromise and I hope you keep pushing and stirring where the UX is less than optimal so we can all make ESPEasy easy to use.

My main problem is that I often don't see the problems myself so you have to keep pushing me to see it :)

[thomastech]

So somewhere we have to find a good compromise and I hope you keep pushing and stirring where the UX is less than optimal so we can all make ESPEasy easy to use.

Thanks for the friendly invitation. I prefer to be helpful and hopefully my occasional pushing is taken as a gentle nudge.

• Thomas

[TD-er]

Looked into the code and it does seem like a very simple fix.
However, I am not 100% sure if there may be side effects I cannot think of now.

The problem was that it looked for "if ", and I changed it into "if" (removing the required space)
So it may now also match things with if in the words.
Maybe you can also think about the possible implications?

[uzi18]

elseif should be also modified respectively

[TD-er]

Just added it.

[thomastech]

The problem was that it looked for "if ", and I changed it into "if" (removing the required space)
So it may now also match things with if in the words.

The only "if" case that would deserve this special treatment would be if[ (if appended with open bracket). So both "if " and "if[" (and elseif) would be valid.

• Thomas

[TD-er]

It could also be used with system variables, which have been replaced by the time it is parsed.
Also the [taskname#varname] has already been parsed by the time this is processed.
So where we try to figure out whether it needs to match a condition, we simply cannot predict what character will be present at that position.

On the other hand, if we were to add a space in front of replaced system variables or [taskname#varname] strings, we have to know the context or else strings sent to a display or URL may get mangled.

[TD-er]

Did anyone test the rules using this patch?
Does it break anything else?

[thomastech]

I'll try it on a NodeMCU if you post the [Testing] bin.

• Thomas

[TD-er]

@thomastech
ESPEasy_mega-20200410-94-PR_3005.zip

[uzi18]

don't see any impact on code but it is less readable and we should avoid such syntax

[TD-er]

don't see any impact on code but it is less readable and we should avoid such syntax

I agree on the syntax stuff, and we should later add warnings, etc.
But for now, make sure we don't break anything.

[thomastech]

I installed ESP_Easy_mega-20200410-94-PR_3005_test_ESP8266_4M1M_VCC.bin on a NodeMCU. But bad news, it breaks the rule handler.

Some events don't run. I deleted all "If" statements and the affected events still don't fire.

I reverted back to ESP_Easy_mega-20200310_test_ESP8266_4M1M_VCC.bin and everything is working again.

It's unexpected that the "if" patch caused this. Maybe there were other changes in this build?

• Thomas

[TD-er]

Do you have some examples of events that no longer were running?

[uzi18]

is it possible for you to attach rules and comments precisely what works and what not?
did you tried to add space char after if?

[thomastech]

Rule file code posted below. The Notification rule (timer4) and NEXTION#idx=98 do not fire. But I can I see Nextion's idx=98.00 in the web log, so this event should occur.

on System#Boot do
  timerSet,4,45
endon

on Rules#Timer=4 do
  Notify 1
endon

on WASHER#ac do
  if [WASHER#ac]=1
    NEXTION,'page0.va_WasherAC.val=1'
  else
    NEXTION,'page0.va_WasherAC.val=0'
  endif
endon

on DRYERMOV#detect do
  if [DRYERMOV#detect]=1
    NEXTION,'page0.va_DryerAC.val=1'
  else
    NEXTION,'page0.va_DryerAC.val=0'
  endif
endon

on NEXTION#idx do
  if [NEXTION#idx]>=10 and [NEXTION#idx]<=30
      Publish /%sysname%/NEXTION/idx,[NEXTION#idx]
  endif
  if [NEXTION#idx]>=500  // Touch Events
      Publish /%sysname%/NEXTION/idx,[NEXTION#idx]
      Publish /%sysname%/NEXTION/value,[NEXTION#value]
  endif
endon

on NEXTION#idx=97 do
  NEXTION,'page7.t_time.txt="Time %syshour_0%:%sysmin_0%:%syssec_0% "'
  NEXTION,'page8.t_ram.txt="Free Ram [FREERAM#ram]"'
  NEXTION,'page8.t_load.txt="System Load [SYSLOAD#load]%"'
endon

on NEXTION#idx=98 do
  NEXTION,'page7.t_wifi_ssid.txt="SSID: %ssid%"'
  NEXTION,'page7.t_ip.txt="IP: %ip%"'
  NEXTION,'page7.t_signal.txt="RSSI: [RSSI#signal]dBm"'
  NEXTION,'page7.t_date.txt="Date %sysmonth_0%/%sysday_0%/%sysyears%"'
  NEXTION,'page7.t_time.txt="Time %syshour_0%:%sysmin_0%:%syssec_0% "'
  NEXTION,'page7.t_uptime.txt="Uptime [RUNTIME#days]days"'
endon
// EOF

• Thomas

[thomastech]

I should mention that these rules have been in use for a couple years on a variety of ESPEasy releases. A few weeks ago I added the single quotes around the Nextion printing statements (as required by the recent releases). But other than that the rules have been working trouble free.

• Thomas

[TD-er]

Just to be sure, the last nightly build does work fine on these rules?

[thomastech]

Affirmative. Rules are working on ESP_Easy_mega-20200410_test_ESP8266_4M1M_VCC.bin

• Thomas

[ghtester]

Could you please also check if Rules Set 2-4 are accessible in that build? I have an issue in Custom build described here: #2980

[TD-er]

Affirmative. Rules are working on ESP_Easy_mega-20200410_test_ESP8266_4M1M_VCC.bin

Check!
That does save me from browsing through a lot of commits to find the cause.

@ghtester It should be fixed as I did fix that +other related issues with toggling some combobox items that would trigger a reload, like switching DNS/IP in controllers, selecting a (new) plugin and some more.

[TD-er]

@thomastech Can you at least also test with the current state of the mega branch?
So then we know if it is just this PR, or maybe something else we (read: I) have broken.

[thomastech]

Can you at least also test with the current state of the mega branch?

The seven day old ESP_Easy_mega-20200410_test_ESP8266_4M1M_VCC.bin is the latest nightly build I can find. As reported, this bin is working.

Do you have a more current [Testing] build for me to try?

• Thomas

[TD-er]

@thomastech Made a test build of the current HEAD of the mega branch.
ESPEasy_mega-20200410-95-g515ba7b5.zip

[thomastech]

Thanks for the test build. Good news, it works.

I checked rules on these two bins:
ESP_Easy_mega-20200410-95-g515ba7b5_test_beta_ESP8266_4M1M.bin
ESP_Easy_mega-20200410-95-g515ba7b5_test_ESP8266_4M1M_VCC.bin

• Thomas

[TD-er]

OK, so it is only the PR I made.
I have been thinking about something else we discussed about this, to warn users on sub optimal syntax.
Well in order to warn, you must detect it and when you detect it, you can also patch it.

So I was thinking about using exactly that on the processing of a single line in the rules parsing.
So stay tuned (not going to happen today though)

[TD-er]

OK, couldn't resist.... at least it is already "tomorrow" ;)

Started a test build for it, which I will link after sleeping, which I will do now.

[TD-er]

Test build: ESPEasy_mega-20200410-101-PR_3005.zip

[thomastech]

Thanks, new bin tested.

1. The rules are working in the test build. I used this bin:
   ESP_Easy_mega-20200410-101-PR_3005_test_ESP8266_4M1M_VCC.bin

2. I edited an "if [" statement so that it was "if[" and confirmed a warning appears in the web log.

The warning message is a bit cryptic. If I was a casual observer I don't believe I would have understood the significance of it. That is to say, would a typical user think it was a basic log status message, warning, or error?
Here is an example:
543276: Rules: 'if[' => 'if [' in: 'if[NEXTION#idx]>=10 and [NEXTION#idx]<=30'

To reduce confusion, if a syntax issue has been automatically corrected then the log message could say something like this:
543276: Rules (Syntax Warning, auto-corrected): 'if[' => 'if [' in: 'if[NEXTION#idx]>=10 and [NEXTION#idx]<=30'

• Thomas

[TD-er]

Thanks for testing and good to hear it is working.
Indeed there should be a more descriptive text, but like I said it was already past my intended bed time...

So it should be an "error" type, as strictly speaking it is not according to our grammar rules.
Do you have any other frequently occurring mistake?

[thomastech]

Do you have any other frequently occurring mistake?

None that I can think of at the moment. Thanks for asking.

• Thomas