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

MQTT topics problems in latest mega branch #2866

Closed
crnjan opened this issue Jan 29, 2020 · 8 comments · Fixed by #2864
Closed

MQTT topics problems in latest mega branch #2866

crnjan opened this issue Jan 29, 2020 · 8 comments · Fixed by #2864
Labels
Category: Controller Related to interaction with other platforms Type: Bug Considered a bug

Comments

@crnjan
Copy link
Contributor

crnjan commented Jan 29, 2020

Hi!

I'm working on a new plugin and noticed strange behaviour in latest develop (while being fine in last pre-release).

Long story short - when using CPLUGIN_005 (Home Assistant (openHAB) MQTT) to publish messages to broker, in latest develop the topic is not correct.

I.e. in my setup the topic should be

"ESP_Easy/xxx/MitsubishiHeatPump"

but its send as

"ESP_Easy/xxx/MitsubishiHeatPump "

(notice the space at the end). I checked what could be the issue and my guess is that change here is the problem.

So parseTemplate pads spaces at the end to match length:

  // padding spaces
  while (newString.length() < lineSize) {
    newString += ' ';
  }

but when we call the parseTemplate from parseControllerVariables, it still contains the placeholder %sysname%:

"%sysname%/xxx/%valname%"

which is replaced with "ESP_Easy" within the parseTemplate function and since "ESP_Easy" has 8 chars (compared to "%sysname%" with 9 chars), parseTemplate will append a space to the end:

"ESP_Easy/xxx/%valname% "

(notice the space at the end). Publisher will use this topic (with extra space) to publish the message ...

Hopefully my explanation makes sense :)

@crnjan crnjan changed the title MQTT topics problems in latest develop MQTT topics problems in latest mega branch Jan 29, 2020
@giig1967g
Copy link
Contributor

I never noticed it, probably because my broker trims the spaces, but it's indeed happenning.
One of my topic is: "ESPT12/d15/State<+ 3 spaces>"

@giig1967g
Copy link
Contributor

@crnjan good catch by the way

@giig1967g
Copy link
Contributor

this will fix it:

  // padding spaces
  while (newString.length() < lineSize) {
    newString += ' ';
  }
  STOP_TIMER(PARSE_TEMPLATE);
  checkRAM(F("parseTemplate3"));

//ADDING TRIM BEFORE RETURN
  newString.trim();
  return newString;
}

giig1967g added a commit to giig1967g/ESPEasy that referenced this issue Jan 29, 2020
@giig1967g
Copy link
Contributor

see #2867

giig1967g added a commit to giig1967g/ESPEasy that referenced this issue Jan 29, 2020
@crnjan
Copy link
Contributor Author

crnjan commented Jan 30, 2020

Hey! Thx for the prompt response. My only comment about the fix is that topic can actually contain spaces ... not sure if someone uses that in practice, but standard allows them ...

Topic Names and Topic Filters can include the space character

Taken from here.

@TD-er
Copy link
Member

TD-er commented Jan 30, 2020

this will fix it:

  // padding spaces
  while (newString.length() < lineSize) {
    newString += ' ';
  }
  STOP_TIMER(PARSE_TEMPLATE);
  checkRAM(F("parseTemplate3"));

//ADDING TRIM BEFORE RETURN
  newString.trim();
  return newString;
}

It shouldn't be fixed there, but in the place where it is called.
As you can see in the code you quoted, it is done intentional. It was once meant to provide alignment for displaying on a display.
The call to this function also somewhere has a number provided to set the (minimal) string length.

I will make a patch for it, as topics obviously don't need to have trailing spaces, so it makes sense to sanitize the topics in the MQTT related code.

@TD-er TD-er added Category: Controller Related to interaction with other platforms Type: Bug Considered a bug labels Jan 30, 2020
@TD-er
Copy link
Member

TD-er commented Feb 4, 2020

Hey! Thx for the prompt response. My only comment about the fix is that topic can actually contain spaces ... not sure if someone uses that in practice, but standard allows them ...

Topic Names and Topic Filters can include the space character

Taken from here.

When trimming the topic, it removes only the leading and trailing spaces.
I think it is not too limiting to have a requirement not to have a leading or trailing space in a topic or filter, right?

TD-er added a commit to TD-er/ESPEasy that referenced this issue Feb 4, 2020
The extra padding to a minimal line length does only make sense for use on displays and some special formatting.
So the majority of the uses of parseTemplate now uses 0 as minimum line length.

Fixes: letscontrolit#2866
Closes: letscontrolit#2867 (superseeds PR)
@crnjan
Copy link
Contributor Author

crnjan commented Feb 7, 2020

I too think that topics with trailing/leading spaces are not really used in practice. Thx for the fix, will give it a spin over the weekend!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Controller Related to interaction with other platforms Type: Bug Considered a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants