[mqtt-things] Homie properties with "$retained"=false attribute are not correctly handled

[bodiroga]

It's me again, the official Homie implementation tester :smile:

I have tried to test the non-retained Homie properties (which should be modeled as Trigger channels in ESH) and they don't behave as expected. I don't see any ChannelTriggeredEvent entry in my log file when I publish a message to the corresponding topic.

Property information:

• "homie/homie-3-sensor/temperature/button/$name" -> "Thermostat Button"
• "homie/homie-3-sensor/temperature/button/$retained" -> "false"
• "homie/homie-3-sensor/temperature/button/$datatype" -> "enum"
• "homie/homie-3-sensor/temperature/button/$format" -> "SHORT_PRESSED,DOUBLE_PRESSED,LONG_PRESSED"

Message sent:

• "homie/homie-3-sensor/temperature/button" -> "SHORT_PRESSED"

But nothing appears in the log file.

IMO, this channel should be described, using XML syntax, as:

<channel-type id="trigger-channel">
    <kind>trigger</kind>
    <label>Thermostat Button</label>
    <event>
        <options>
            <option value="SHORT_PRESSED">pressed</option>
            <option value="DOUBLE_PRESSED">double pressed</option>
            <option value="LONG_PRESSED">long pressed</option>
        </options>
    </event>
</channel-type>

[NOTE: Although the ideal solution would be to model it as a system.button trigger channel (more info here).]

Anyway, the main goal of my message is to point that the channel is not behaving as a trigger channel. The representation of the channel in the Paper UI is not correct, here you have an imagen of how it looks like:

Comparing the channel to the trigger channels in the astro binding, it shouldn't have an item-type field (the 'String' at the bottom of the channelUID), because it can't be linked to an item (well, that's not entirely correct, they can be linked to an item with a profile, but this is not supported in the binding).

Also, having a look at the source code (the part related to the Homie implementation), I haven't found any "triggerChannel" call, which is the function used to fire an event in a trigger channel. All the channel updates are handled by the AbstractMQTTThingHandler class, but that class treats all channel updates as state updates with the updateState function. This is, AFAIK, the main problem: trigger channels are not correctly updated using the triggerChannel function.

What do you think @davidgraeff? Is the triggerChannel function used in the Homie code? Is there anything I haven't taken into account?

Your work in the Mqtt binding is amazing, thanks you sooo much!

Best regards,

Aitor

[davidgraeff]

I confirm this is undesired behaviour.

[davidgraeff]

This is fixed with the last PR. @kaikreuzer

[bodiroga]

Sadly the latest openHAB snapshot version (#1426) doesn't include your recent changes, I can see that the ids of the mqtt bundles are 0.10.0.201811151037. I will post a new message to confirm that everything works as expected, although I'm pretty sure that David has fixed the problem :+1:

[davidgraeff]

@bodiroga See #6523

[bodiroga]

Hi @davidgraeff! Yeah, I saw that PR yesterday and I'm aware that it was merged at night, but sadly the latest openHAB snapshot version from today doesn't include it. That' why I cannot test it right now. It is just a mere formalism, I like to confirm that the bugs reported by me have been fixed in my installation :wink: Awesome job as always!

[kaikreuzer]

@bodiroga Please see https://github.com/eclipse/smarthome/wiki/ESH-used-in-openHAB.

[bodiroga]

Ouch, I didn't know that Kai, sorry! I believed that the process was automated, thanks for the information! :+1:

Anyway, I have pulled the latest version of the ESH repository to my Eclipse IDE setup (commit id: 7875d6f994a3382bf3f8fd13e8616697d270da72) and I can't confirm that the issue has been solved. I don't see any ChannelTriggeredEvent event in the Console, while the rest of the Homie device channels update their item states accordingly. The channel still has the "itemType: "String" attribute, although I'm not sure if that's the problem. I haven't linked the channel to an item (it doesn't work, anyway, but the "Link channel" pop-up appears in Paper UI), because, AFAIK, Trigger channels don't need it, they should emit events without any user interaction.

I will try to debug the problem more and come back with my results. @davidgraeff, did you perform any test to confirm that the bug should be resolved?

Thank you guys!

[kaikreuzer]

@bodiroga No worries, I know that the build chain is pretty complex ;-) Fyi, I have just created a new ESH stable, so have a try with openHAB distro 1427 once it is finished.

[bodiroga]

Thanks Kai :+1:

Latest openHAB distro build (1427) shows that the issue hasn't been resolved, sorry @davidgraeff :sob:

I will not paste again my python script, but as I described in my first message, the information published by the "virtual" device is the following one:

homie/homie-3-temperature-sensor/temperature/button/$name Thermostat Button homie/homie-3-temperature-sensor/temperature/button/$retained false homie/homie-3-temperature-sensor/temperature/button/$datatype enum homie/homie-3-temperature-sensor/temperature/button/$format SHORT_PRESSED,DOUBLE_PRESSED,LONG_PRESSED

At the same time, the script sends the payload "SHORT_PRESSED" to the "homie/homie-3-temperature-sensor/temperature/button" topic every 10 seconds to simulate a button press. As the channel should be a trigger channel (that is why the "$retained" attribute was created), I should see a new ChannelTriggeredEvent message in the "events.log" file, but I can't see anything.

Can someone reopen the issue until a new fix is found?

PS: I have added some log messages to the processMessage() function in the ChannelState.java class and it seems that no triggerChannel associated MQTT messages are being passed to the binding, because the function is not triggered. The MQTT message arrives correctly to the embedded broker (Sending PUBLISH message. MessageId=0, CId=mosqsub/10581-aitor-por, topic=homie/homie-3-temperature-sensor/temperature/button in the console), but nothing else. It seems that the binding doesn't subscribe to the state topic (homie/homie-3-temperature-sensor/temperature/button).

[bodiroga]

Ok, I know where the problem is, although I don't know how @davidgraeff wants to address it.

In the Property.java file -> ".createChannelFromAttribute()" method, at line 168 (link), when the property is non-retained, the ChannelConfigBuilder doesn't get any topic because it's not retained nor settable. That makes the ChannelState not have any topic to subscribe to :disappointed: But giving a state to the ChannelConfigBuilder with the ".withStateTopic()" method doesn't work because the ChannelState class, in its constructor (link), checks if the stateTopic is blank to determine if we have a trigger channel.

My proposal:

• Set always the stateTopic in the ChannelConfigBuilder in createChannelFromAttribute() method:
  • ChannelConfigBuilder b = ChannelConfigBuilder.create().withRetain(attributes.retained).withStateTopic(stateTopic);
• Change the trigger attribute evaluation in the ChannelState constructor:
  • this.trigger = !config.retained;

What do you think @davidgraeff? Is this an acceptable solution for you? I have tested the changes and everything works fine! :+1:

Anyway, we should also address the problem that trigger channels should not be allowed to be linked to regular items. This can be easily implemented adding the following code to the previously named ".createChannelFromAttribute()" method:

        if (attributes.retained) {
            this.channel = ChannelBuilder.create(channelUID, channelState.getItemType()).withType(channelTypeUID)
                    .withKind(type.getKind()).withLabel(attributes.name)
                    .withConfiguration(new Configuration(attributes.asMap())).build();
        } else {
            this.channel = ChannelBuilder.create(channelUID, null).withType(channelTypeUID).withKind(type.getKind())
                    .withLabel(attributes.name).withConfiguration(new Configuration(attributes.asMap())).build();
        }

Thanks for your patience and help and best regards,

Aitor

[davidgraeff]

I know why. A trigger channel doesn't have a state. So I do not pass the state topic. But then the channel cannot subscribe of course. To fix my own confusion, I'll rename the "stateTopic" variable to "stateOrTriggerTopic".

[bodiroga]

I know why. A trigger channel doesn't have a state. So I do not pass the state topic. But then the channel cannot subscribe of course. To fix my own confusion, I'll rename the "stateTopic" variable to "stateOrTriggerTopic".

I was 30 seconds faster than you! :rofl: You also need to change the "trigger" attribute initialization in ChannelState's constructor, because now you are checking if (the future) stateOrTriggerTopic is empty. A simple "!config.retained" expression will be fine :+1:

[davidgraeff]

@bodiroga No that is not enough unfortunately. This same class is used for Generic and HomeAssistant as well. And the generic channels do not have a config "retained".

[bodiroga]

@davidgraeff Ok, understood. The problem relies in the fact that the word "retained" means different things for the Homie convention and for the ChannelConfig class, right? I don't understand very well what does the "retained" attribute in the ChannelConfig class represent. In this class, the attribute has the following description: /** If true publishes messages as retained messages */ and, for me, that means that outgoing messages will be sent as retained. At the same time, the ChannelState class has a method call "isStateful()" which, relying on the same retained attribute, defines if the channel is a stateful channel, but for me stateful/stateless means that the incoming message is retained or not. It seams that the binding is using the same attribute to represent information about incoming and outgoing messages :confused:

Generic channels do have the retained config flag and by default it is set to "false". This retained config variable is described as "The value will be published to the command topic as retained message. A retained value stays on the broker and can even be seen by MQTT clients that are subscribing at a later point in time." in Paper UI.

Is a simple "retained" attribute enough to represent all available combinations? :thinking:

[davidgraeff]

Oh true, I have added retained to the generic channels :confused: No a single boolean flag is of course not enough to represent 4 possible states. But for the generic channels it is just absolutely not interesting if the incoming message is retained or not. And for Homie it's the same.

An additional boolean "isTrigger" for the constructor should be enough. The config.retained is for the publishing part only and is not involved in deciding if it is a trigger channel.

[bodiroga]

Oh true, I have added retained to the generic channels

Great, I thought I was missing something :laughing:

No a single boolean flag is of course not enough to represent 4 possible states. But for the generic channels it is just absolutely not interesting if the incoming message is retained or not. And for Homie it's the same.

Why? I know that you have introduced triggerChannels in the binding through the broker thing, but I'm not sure if that is the best solution. Imagine I have a temperature sensor that has a generic external button and that I want to use that button as a trigger device in my system. Why can't we allow users to model this device as a generic thing with two channels: a Number value with a stateTopic and a new Trigger with a stateTopic or triggerTopic. It feels awkward to model this as a generic thing with a single channel (for the temperature) and to add the button to the broker thing. Using the Homie convention I am allowed to model this under the same device (Homie thing), but for non-Homie devices it is impossible. It would be great if you could reconsider this, it fits perfectly into the binding architecture with small changes.

An additional boolean for the constructor "isTrigger" should be enough. The config.retained is for the publishing part only and is not involved in deciding if it is a trigger channel.

The isTrigger (or trigger) attribute should be added to the ChannelState or to the ChannelConfig class? For me the second option feels better, but from your sentence I understand that you want to add it to the first one. The default value, in both cases, will be false.

The ".createChannelFromAttribute()" method in the Property class could be changed to:

ChannelConfigBuilder b = ChannelConfigBuilder.create().withTrigger(!attributes.retained).withStateTopic(stateTopic);

if (attributes.settable) {
    b = b.withCommandTopic(commandTopic);
}

final ChannelState channelState = new ChannelState(b.build(), channelUID, value, callback);
this.channelState = channelState;

Notes:

• The previous .withRetain(attributes.retained) call is removed, in Homie "retained" talks about incoming messages, not about the ones to be published. No Homie property needs to set retained to true, because the convention doesn't say that "/set" messages must be sent with retain flag.
• .withTrigger(!attributes.retained) feels really strange name wise, but it's ok.
• All properties will have a stateTopic, so the "if (attributes.retained)" condition goes away.
• The this.trigger initialization in ChannelState's constructor will change from this.trigger = StringUtils.isBlank(config.stateTopic); to this.trigger = config.trigger;.

Do you agree?