rkoshak / sensorReporter

A python based service that receives sensor inputs and publishes them in various ways.
Apache License 2.0
105 stars 41 forks source link

Do not publish actuators from PollManager report #95

Closed rkoshak closed 1 year ago

rkoshak commented 2 years ago

@DanielDecker, before I submit a PR to take care of this I want to make sure I understand what's going on.

This morning at 06:24 my sensorReporter restarted. During startup PollMgr.report was called and your new additions to publish the states of Actuators ran. So far, so good. But I think there is a bug in rpi_gpio.py.

    def publish_actuator_state(self):
        """Publishes the current state of the actuator."""
        msg = "ON" if self.current_state else "OFF"

        #publish own state, make sure the echo gets filtered
        self._publish(msg, self.cmd_src, filter_echo=True)

Shouldn't that publish to self.destination instead of self.cmd_src? By publishing to cmd_src it's causing the actuator to trigger which, at least in this one case, is absolutely not desirable. It make sense to report the current state, but not trigger the action.

DanielDecker commented 2 years ago

I don't think there is a bug. The idea behind this routine is to make sure the remote connections (mqtt, openHAB) show the correct status for the actuator. e.g.

[Actuator_LED]
....
CommandSrc = red_led 
# (internal self.cmd_src = CommandSrc)
InitialState = ON

On startup sensor_reporter will switch this LED on and send the status to 'red_led' on the configured remote connections. To avoid that sensor_reporter triggers the same actuator (e.g. red_led), I introduced the parameter filter_echo. When set to True the connection will ignore the command for this actuator which it has send just before. The same happens if the sensor update was triggered manually via the poll_mgr.report() function.

rkoshak commented 2 years ago

The idea behind this routine is to make sure the remote connections (mqtt, openHAB) show the correct status for the actuator.

Right, but the status topic is self.destination. That's where sensorReporter publishes the states. self.cmd_src is the topic that sensorReporter subscribes to for commands. When a command is received there, the actuator triggers it's action (based on the message published). If configured, the actuator then publishes the new state of the action to self.destination.

By publishing to self.cmd_src sensorReporter is basically commanding itself and by not publishing to self.destination nothing else is getting the state update. If anything get's published to self.destination at all it's as a side effect of the command.

I don't think that sensorReporter should be commanding itself and taking actions like this just because it's reconnected to the broker. Connecting/reconnecting to the broker should be idempotent. It should only publish the latest status, not take actions. In some cases, taking an action like that is flat out wrong, for example in cases where the actuator is a toggle.

DanielDecker commented 2 years ago

By publishing to self.cmd_src sensorReporter is basically commanding itself and by not publishing to self.destination nothing else is getting the state update. If anything get's published to self.destination at all it's as a side effect of the command.

The filter_echo makes sure sensor_reporter is not commanding itself. And openHAB will get the status update even when publishing to self.cmd_src.

Before I handed in the PR for this I was facing following problem: A switch item in openHAB connected with sensor_reporter shows not the current status for a given RpiGpioActuator after the start of sensor_repoter (InitialState = ON) or after that actuator state was changed via another connection e.g. a local connection. To me the proper solution is to change the switch item directly, so it shows the actual status of the RpiGpioActuator.

Would you configure OH to have a status item and a switch item for every light (x 20 per house)? This would also result in pressing the switch (in openHAB) twice when it got stuck on ON but you want to switch the light ON.

In some cases, taking an action like that is flat out wrong, for example in cases where the actuator is a toggle.

I agree with you for Toggle = true or SimulateButton = true. But even here sensor_reporter is ignoring the "echo".

For clarification you don't like:

  1. RpiGpioActuator publishes it status to self.cmd_src
  2. the status report is triggered on Connecting/reconnecting

Do I understand you right?

rkoshak commented 2 years ago

The filter_echo makes sure sensor_reporter is not commanding itself. And openHAB will get the status update even when publishing to self.cmd_src.

Then there is a bug there for sure, as evidenced by the fact that my garage doors opened at 5:45 AM when for some other reason my Mosquitto went down. When sensorReporter reconnected, it went through that poll manager code to publish and the filter did not block the command. Note that I can't make this happen consistently so I suspect there is a timing or order of operations problem with the filter that needs to be looked at.

And even if the filter did block it, I still maintain that sensorReporter has no business publishing to its own command topic in that case. It should be publishing to the state topic.

A switch item in openHAB connected with sensor_reporter shows not the current status for a given RpiGpioActuator after the start of sensor_repoter (InitialState = ON) or after that actuator state was changed via another connection e.g. a local connection.

In all of those cases, the actual state of the actuator belongs in the destination topic, not the command topic. To avoid infinite loops the state and command topics are kept separate. sensorReporter always and only publishes the state to the destination topic. sensorReporter always and only subscribes to messages on the command topic (note I'm only talking about an individual Actuator, having another actuator or sensor send a command to the command topic is OK, initialization is OK too).

To me the proper solution is to change the switch item directly, so it shows the actual status of the RpiGpioActuator.

Yes, by subscribing to the destination topic and publishing to the different command topic. You need both and you need to keep them separate.

Would you configure OH to have a status item and a switch item for every light (x 20 per house)?

No because the MQTT binding supports two topics per Channel, a subscription and a command.

image

Messages on the state topic result in an update (or command if configured that way) on the Item. Commands sent to the Item result in openHAB publishing a message to the command topic.

When you use the same topic for both you end up with infinite loops because MQTT doesn't know or care that the same client is subscribed and publishes to the same topic.

I agree with you for Toggle = true or SimulateButton = true. But even here sensor_reporter is ignoring the "echo".

But it isn't or else I wouldn't have opened this PR in the first place. Maybe it's a matter of timing but this discussion makes me wonder if the whole approach is perhaps not the best if you were previously unaware of how MQTT Things work with two topics on the same Channel. Maybe the filter isn't even required. I'm also wondering if using retained messages might be a better approach over all for this initialization and reporting.

Do I understand you right?

I'm not sure.

  1. Yes it is true that I don't like publishing to self.cmd_src. The current state of the Actuator should be published to self.destination. That's the state topic. An Actuator shouldn't be commanding itself to do something just because it reconnected to the Broker.

  2. I have no problem with status reporting on reconnecting. I think that's a good idea actually. But the status topic is self.destination, not self.cmd_src. Publishing to self.cmd_src isn't actually reporting the status. The only thing that should be subscribed to that topic is the one sensorReporter Actuator (or many if you have one topic to command more than one Actuator, though in that case each Actuator should still have its own destination, or somehow messages to indicate which sensor is changing state). Everything else should be subscribed to the destination topic to get updates on the Actuator's current state.

I should look like this:

image

A round trip might look like the following:

  1. something sends a command to the openHAB Switch
  2. the MQTT binding activates on the command
  3. the Channel linked to the Switch Item has mylight/cmd configured as the topic. The binding publishes ON to mylight/cmd
  4. sensorReporter is subscribed to mylight/cmd and receives the ON command
  5. the actuator subscribed to mylight/cmd gets the ON command and does what it's configured to do with that message
  6. the actuator enters a new state, therefore it publishes that new state ON to self.destination, which in this example we'll call mylight/state
  7. the MQTT binding is subscribed to mylight/state and receives the ON message
  8. the binding sends an update to the openHAB Switch Item so it represents the new state

When you turn of autoupdate on the Item, the Item won't actually change state to ON even at step 3 until the ON message is received and processed at step 8 ensuring that openHAB doesn't assume the light is ON until it actually reports that it's ON, which is the ideal since we want OH Items to reflect the device's actual state when possible rather than guessing.

If something else can command that Actuator in sensorReporter, let's say it's a physical button, then jump to step 6. The sensorReporter actuator changes state, publishes the new state which OH receives and updates the Item as appropriate.

When OH reconnects to the MQTT broker, again we only want to start at step 6 and have sensorReporter report the current states, not command the actuators to take an action they don't really need to.

DanielDecker commented 2 years ago

Thank you for the detailed explanation!

After reading your first comment I didn't understood you had a malfunction with a RpiGrioActuator. Now you made it clear that the actuator may trigger unexpected during reconnect. From you're illustration of the MQTT binding I see know that I wasn't using it properly. I agree with you, that when publishing the actuator state to the "command"-topic cause a risk that it will trigger the same actuator again. I developed the MQTT message filter and tested it in detail to make sure this would not happen.

After reading thru your explanation I fully agree with your initial comment. The actuator should not report it's status to the same topic. But when coding this I had also the openHAB REST API in mind, where a command topic and a destination topic mean two separate items. Am I right? So when I asked:

Would you configure OH to have a status item and a switch item for every light (x 20 per house)?

I actually thought about the OH REST API.

If I understand well openHAB REST API doesn't use different "topics" to differ between command and status. Instead the same "topic" is used and OH will interpret it as a command if the POST-method is used and as a status update if it is the PUT-method. sensor_reporter always uses the PUT-method so OH will receive everything as status update.

Currently OH will receive the status update (via REST API) from sensor_reporter correctly. Simply replacing self.cmd_src with self.destination will break this. One solution to fix this for both connections, MQTT and REST API, would be to change (in connection.py):

@abstractmethod
def publish(self, message, destination, filter_echo=False):

to

@abstractmethod
def publish(self, message, destination, command_src=None):

and decide on connection level where the status update will get published. This would also make the filter unnecessary. What do you thing about that solution?

rkoshak commented 2 years ago

But when coding this I had also the openHAB REST API in mind, where a command topic and a destination topic mean two separate items. Am I right?

Honestly, I am not sure. I never really used the openHAB REST connection. It should work the same but I can't say for sure that it does. In this case, the same Item name would be used for both the cmd_src and the destination in the config ini. The actuator would listen for command events on that Item and post updates when the state changes. It would just be slightly annoying to have to list the same Item twice in the config, but I think it's a small price to pay to maintain the consistency.

And in cases where we don't care about the actuator reporting it's state, leave out the destination and nothing gets published.

If I understand well openHAB REST API doesn't use different "topics" to differ between command and status.

True but the two are different types of events. OH has three Item events that mean different things and are processed in different ways.

So you can use the same Item as both the cmd_src and destination because instead of using separate topics to avoid the infinite loop, we listen for/publish different events. The actuator listens for commands and only posts updates. The picture looks like this (I happened to have the MQTT drawing from above still open :-D ).

image

Of course there is the option to use a different Item, but we don't have to. We can use the same Item for both destination and cmd_src.

sensor_reporter always uses the PUT-method so OH will receive everything as status update.

Yes, this is by design per above.

Simply replacing self.cmd_src with self.destination will break this

Not replace. You need both, though destination can be optional (e.g. in cases where the actuator is a momentary button without a real state).

What do you thing about that solution?

The way I've fixed it in my code for now is to change the rpi_gpio.py.

    def publish_actuator_state(self):
        """Publishes the current state of the actuator."""
        msg = "ON" if self.current_state else "OFF"

        #publish own state, make sure the echo gets filtered
        if self.destination is not None:
            self._publish(msg, self.destination, filter_echo=True)

If the self.destination is defined, publish the current state to the destination. Otherwise do nothing. That way if there is a destination defined, we always publish there. We assume that there always is a cmd_src, otherwise the actuator wouldn't have a point.

What I don't know is what that does for the filter. I think it eliminates it entirely but I admit I don't understand that part very well so can't say for sure. It seems like it could since it doesn't really matter if we publish updates to the destination, whether it's through the REST API or MQTT. Updates like that are to be expected and they won't generate an infinite loop. But that of course only work if the openHAB connection only subscribes to commands on the Item which I need to verify which appears to be the case.

            # See if this is an event we care about. Commands on registered Items.
            decoded = json.loads(event.data)
            if decoded["type"] == "ItemCommandEvent":
DanielDecker commented 2 years ago

You're fix is simple and works obviously with the MQTT connection, it will also work with the openHAB REST connection (when cmd_src = destinationas you suggested). But isn't sensor_reporter build to have multiple connections?

I actually plan a setup which involves MQTT and openHAB REST at the same actuator. At your last comment you explained in long why a MQTT msg round trip should have 8 steps and why it is bad for a MQTT connection to publish to is own cmd_src. I fully agree with this.

When using both connections (MQTT and openHAB REST) at the same actuator, I would have to set cmd_src = destination to make the openHAB item work properly. But this will break the msg round trip for MQTT and maybe someday my garage door would also open unexpectedly. :-P That is why I don't like the simple fix.

I can hand in a PR that will fix it that way:

def publish_actuator_state(self):
        """Publishes the current state of the actuator."""
        msg = "ON" if self.current_state else "OFF"

        #publish own state, toggle = False
        if not self.sim_button:
             self._publish(msg, self.destination, self.cmd_src)

_publish implemented in the local connection would only use the destination, after checking it's not None and ignore cmd_src, the same goes for the MQTT connection. The openHAB REST connection would publish to cmd_src if provided (a sensor will not provide a cmd_src to publish) otherwise to destination. Since this solution would never publish to its own cmd_src I would also remove the filter_echo code, which is not needed anymore. You still don't like that solution?

Just to explain the filter for your fix:

What I don't know is what that does for the filter.

When destination != cmd_src the filter will do nothing. When destination == cmd_src the filter will trigger and try to filter the own commands, as you reported this doesn't work in all cases.

Btw. this project cough my interest because of the openHAB REST connection. So I'm eager to keep the REST API on the same level as the MQTT connection. ;-)

rkoshak commented 2 years ago

But isn't sensor_reporter build to have multiple connections?

This is only partially implemented. The problem is the actuator only supports defining one "CommandSrc" parameter and one "Destination" parameter. As long as that source and destination makes sense to all of the communicators you are fine. But it doesn't for MQTT and openHAB REST API. For MQTT it's a topic that looks like sr/garage/garagedoor1/src and for openHAB it's an Item name that looks like GarageDoor1. Not even the syntax for the two are the same.

To really support this in the way you want to use it the sensor and actuator classes would need to be modified so multiple "CommandSrc" and "Destination" parameters can be provided and each of those can be associated with a given communicator.

That's a missing piece that will be required for you to reach your end goal. It got really complicated the last time I refactored the code so I abandoned that for now. I think moving to YAML, XML, or JSON to define the configuration will be appropriate at that point but that's going to touch everything.

I actually plan a setup which involves MQTT and openHAB REST at the same actuator.

Why? What do you need that one or the other doesn't provide? (not rhetorical, I do want to know the use case) I would expect to use MQTT and then openHAB subscribe/publish to MQTT just like any other service that needs the MQTT also. Or openHAB only and openHAB would forward and handle synchronizing between the devices that need the MQTT.

When using both connections (MQTT and openHAB REST) at the same actuator, I would have to set cmd_src = destination to make the openHAB item work properly. But this will break the msg round trip for MQTT and maybe someday my garage door would also open unexpectedly. :-P

But, like I said, you can't have both MQTT and openHAB REST on the same actuator anyway, not without some significant changes. But with those changes each communicator would have its own source and destination configuration so this wouldn't be a problem.

tl;dr: It wouldn't make sense to use both an MQTT and openHAB REST connection on the same sensor/actuator until other changes are made. Once those other changes are made, this won't be a problem.

You still don't like that solution?

I have one major problem with it. I'd much rather add true support to use both the MQTT and openHAB REST communicators by adding the ability to define separate CommandSrc and Destinations for each communicator. When that happens, then we don't need to add any special logic or work arounds or stuff like that. Both can work consistently and we can have a simple "only publish to destinations, only subscribe to cmd_srcs" rule.

Then there is no need for the filter (as I now understand it) and the two communicators can behave consistently so there is no need for special logic in core or the actuators/sensors to handle one of the communicators working differently.

Btw. this project cough my interest because of the openHAB REST connection. So I'm eager to keep the REST API on the same level as the MQTT connection. ;-)

This is actually one of my major driving principals. But that means keeping the two communications as similar and consistent with each other as possible. I think we achieve that by simply requiring the user to configure a destination even when using the openHAB REST connection rather than adding a bunch of special logic that only is meaningful for the openHAB REST connection.

There is no logic for any type of communicator to prevent using the same value for cmd_src and destination.

DanielDecker commented 2 years ago

Why? What do you need that one or the other doesn't provide?

I plan following setup: one or two sensor_reporter for each room. Each sensor_reporter is wired with the light switches, motion detectors, lights and switchable power outlets of that room. I will use local connections to connect motion detectors with lights. I want to the MQTT connection to connect the sensor_reporters with each other. To connect all actuators and some of the sensors with OH I would use the REST API.

I choose MQTT for the connection between sensor_reporters because it's is simple and therefor I expect it to be more reliable then redirecting it via OH. Furthermore I simply can control a sensor_repoter actuator with a sensor when using the same topic and values (values = OFF,ON). While with OH I'd have to create a rule which will watch for a state update to send a command to make a sensor_reporter actuator switch ON or OFF. I choose the REST API for the connection to OH because it is a little bit easier to setup. I don't need to configure the Generic MQTT Thing for each topic and therefore save time and a potential error with typos. This sounds probably incomprehensible but consider ~20 sensors/actuators per room with 5 or 6 rooms equip with sensor_reporters. So in both cases I choose easier solution.

But, like I said, you can't have both MQTT and openHAB REST on the same actuator anyway, not without some significant changes.

Here I have a simple fix. Actually I run sensor_reporter in dual mode (MQTT + REST API) for months. I didn't changed the source code. I had just to break the naming convention of MQTT. To be honest when it configured my sensor_reporter test setup I didn't know hat I have to use a slash ("/") in the topic name. Is it a must or a recommendation? So I used for destination and cmd_src topics like ground-floor_kitchen_main-light or in short gf_k_main-light which will work for both connections. If I understand well I lose only the ability to subscribe to subtopics (e.g. ground-floor/#) on the MQTT side, while I gain access to OH via the REST API. I don't take this as a permanent solution but it works for now.

tl;dr: It wouldn't make sense to use both an MQTT and openHAB REST connection on the same sensor/actuator until other changes are made. Once those other changes are made, this won't be a problem.

I can think of a wall switch which will toggle the main light (configured via local or MQTT connection) and OH disabling the light because the "cinema" scene was enabled (via REST API).

To really support this in the way you want to use it the sensor and actuator classes would need to be modified so multiple "CommandSrc" and "Destination" parameters can be provided and each of those can be associated with a given communicator.

I would prefer a solution which will hide the differences of the different connections from the user, to make the configuration as simple as possible. But we can solve this problem in another "issue".

I have one major problem with it. I'd much rather add true support to use both the MQTT and openHAB REST communicators by adding the ability to define separate CommandSrc and Destinations for each communicator. When that happens, then we don't need to add any special logic or work arounds or stuff like that. Both can work consistently and we can have a simple "only publish to destinations, only subscribe to cmd_srcs" rule.

Then there is no need for the filter (as I now understand it) and the two communicators can behave consistently so there is no need for special logic in core or the actuators/sensors to handle one of the communicators working differently.

Yes, in this case there would be no need for the filter_echo. Obviously it's not the only solution to get rid of the filter. I understand that you'd rather add more configuration parameters to keep the source code as simple as possible. I think exactly the other way. =/

I think we achieve that by simply requiring the user to configure a destination even when using the openHAB REST connection rather than adding a bunch of special logic that only is meaningful for the openHAB REST connection.

I don't mind a destination parameter for the RpiGpioActuator. But I think it is inevitable to have special logic for one connection which will have no corresponding part at the other connection, because the REST API and MQTT are quite different. Just think about OH MQTT device discovery. There is no automatic discovery for a device connected via REST API, so there would be the need for a totally different approach.

I think everything boils down to the following statements:

rkoshak commented 2 years ago

I want to the MQTT connection to connect the sensor_reporters with each other. To connect all actuators and some of the sensors with OH I would use the REST API.

It would be simpler to have OH also subscribe/publish to the same MQTT than to add the complexity of managing two connections. Setting up a Generic MQTT Thing is not that difficult. And since these would all be very similar you can copy/paste/edit to create those that are similar either through the code tab in MainUI, the REST API docs, or by using .things files.

It's going to be less work than it will be to get something that works reasonably well in sensorReporter I think, but that doesn't mean we shouldn't improve the support to handle both in sensorReporter too.

Is it a must or a recommendation?

MQTT messages are meant to be hierarchical and supports wild card subscriptions. Not using a hierarchy is an anti-pattern. It'd be like putting all your files in / without using subfolders. You could do that but it is not recommended.

Right now you are considering only your use case of connecting sensorReporters together using MQTT. But what if someone wants to connect to openHAB REST and IOBroker/HomeAssistant/MQTT Dashboard/Tasmota/Shelly, etc. over MQTT? sensorReporter doesn't have full control over the topics that must be subscribed or published to and those will use a hierarchy incompatible with openHAB Item names. In that case we can't use sources and destinations that are the same for both.

Since we will have to support separately defining the source and destination for each communicator anyway why not just do it that way from the start?

I don't take this as a permanent solution but it works for now. If we are going to change sensorReporter to handle this, I'd prefer a permanent solution from the start rather than complicating the core code to handle a special case that is only special because of a temporary solution.

I can think of a wall switch which will toggle the main light (configured via local or MQTT connection) and OH disabling the light because the "cinema" scene was enabled (via REST API).

I'm not sure I understand how this use case is related. That's a different problem to be solved. Having two different communicators or not on the same actuator using the same sources and destinations doesn't address that problem. That's a synchronization of state problem. sensorReporter doesn't have a concept of an actuator being disabled so what ever is happening on the OH side won't prevent another sensorReporter from commanding an actuator over MQTT (for example). Everything would have to go through OH in that case so OH could stop the command if the actuator is disabled, or the actuator would need to support a special command sent to it to enable/disable it. But that's independent of cmd_src and destination concerns. That's just a new feature.

I would prefer a solution which will hide the differences of the different connections from the user, to make the configuration as simple as possible.

I agree and I tried to get as close as possible to that over the years. But I am not willing to do that in a way that forces the use of MQTT in a way that is against accepted best practices or in a way that limits the ability to add communicators in the future.

I still have on my list support for the Homie MQTT standard so OH (and others) can automatically discover the sensorReporter topics and create the Things for you. Support for BTLE, Matter, or other pub/sub standards becomes very challenging to add as well if every communicator has to use the exact same cmd_src and destination configuration and the core logic becomes very complicated handling all the differences between them.

I understand that you'd rather add more configuration parameters to keep the source code as simple as possible. I think exactly the other way. =/

It's not just to keep the code simple, it's so I can add support for more communicators in the future. That's really my primary concern. That and supporting all the standards in that standard's way. Straight MQTT and openHAB REST has never been the only two I want to support. In fact the last time I refactored sensorReporter it was explicitly to get to a point where I could add a Homie connector. There is still a lot that needs to be added and changed to support that but if we start adding stuff to core to support differences in how communicators work I'll be right back where I started and there is no chance to support Homie or anything else.

That seems like a pretty significant hit just to make it so the end user doesn't have to add a couple extra entries to the initial config. It's not an even trade in my book.

But I think it is inevitable to have special logic for one connection which will have no corresponding part at the other connection, because the REST API and MQTT are quite different. Just think about OH MQTT device discovery. There is no automatic discovery for a device connected via REST API, so there would be the need for a totally different approach.

At which point I would do my best to push that into the Communicator if possible. If not another refactor or something might be required. But it's not going to be a problem for OH MQTT device discovery. In that case we still need to specify at a minimum a destination (with the cmd_src being defined by the standard being used). And the logic that handles that would be embedded in the communicator, not something that core needs to know or deal with.

What does need to be added to the Actuator and Sensor classes (possibly) is a way for the Communicator to query for it's capabilities so that it can publish the initial registration message. That indeed is new stuff that would have to be added. But it doesn't change how core sends and receives messages. It would still publish to destination and subscribe to cmd_src. And that new stuff could be reused for Matter support too, which I'd love to add someday.

So we would have two types of communicators, the type we have now which just blindly pub/sub based on the configured cmd_src and destination. The second type would interrogate the sensors and actuators for their capabilities, but the sensors and actuators would still pub to the destination and sub to the cmd_src in the same way no matter what. Figuring out what to do with that is the communicator's job though, not core's.

If it makes sense for this one openHAB communicator to do away with one or the other of cmd_src or destination, to simplify the config, I can get behind that if the changes to handle that are made to the openHAB REST communicator itself instead of the core. By making the changes to core, it makes it difficult to expand to new communicators later.

You intend sensor_reporter as either MQTT or REST API connector, while keeping the code base as close together as possible.

No, I've never said that. Right now it only supports one or the other but I'm willing to see a use for both at the same time. I've always had plans to make it so the two could be used together more seamlessly. But I'm not willing to do so in a way that encourages anti-patterns of use for either connection or that limits the ability to expand with additional communicators in the future.

And I think we can get to both by adding the ability to define separate sources and destinations for each communicator.

I think there is a advantage in using MQTT and REST API at the same time. You say it's enough to use one or the other.

I say that right now the only way to use the two at the same time on the same actuator is to misuse one (MQTT). And at this time it would be less work to just use one or the other, not both.

But I also want to see the ability to support both. But I'm not willing to do so in a way that causes sensorReporter to misuse MQTT and in a way that closes off the ability to add new and more diverse connections in the future. The only way to achieve that is to support separately defining the cmd_src and destination.

And maybe there needs to be an internal broker of some sort. Maybe we need to move the connection configuration away from the actuators and sensors and create a central core class where we define the wiring and the sensors and actuators just emit or receive the messages (kind of like openHAB's event bus). Maybe the configuration needs to be moved to the communicators themselves instead of the sensors and actuators (e.g. the name of the sensor/actuator and source and destination are defined there).

I'm open to any of these sorts of approaches. Anything that doesn't require us to have MQTT antipatterns to handle both at the same time and doesn't close down the possibility to add communicators later.

I think in the short term, the easiest will be to expand the sensor and actuator's config to support separate source and destination per connected communicator.

DanielDecker commented 2 years ago

For clarification: When you write about 'communicators' or 'connector': I assume you mean local_conn.py, mqtt_conn.py and rest_conn.py. Your mention a Homie connector: I assume you intend to leave mqtt_conn.py as it is and have new class which is mqtt_conn.py + Homie standard.

Since we will have to support separately defining the source and destination for each communicator anyway why not just do it that way from the start?

I assume this results from my problem of connecting e. g. a actuator with two different communicators at the same time.

... adding stuff to core to support ...

I assume with 'core' in this context you mean the core classes located in the core directory.

And since these would all be very similar you can copy/paste/edit to create those that are similar either through the code tab in MainUI, the REST API docs

With 'REST API docs' you mean using the 'API explorer' to copy/paste/edit the Generic MQTT Thing?


But what if someone wants to connect to openHAB REST and IOBroker/HomeAssistant/MQTT Dashboard/Tasmota/Shelly, etc. over MQTT?

You're right. I only wanted to point out that it already works without hierarchical messages. I think to get it working properly (with hierarchical msg's) there is only a little work needed. Obviously I was only thinking about MQTT + Homie + REST API, with more connectors there is no way of sticking to one cmd_src and destination.

I can think of a wall switch which will toggle the main light (configured via local or MQTT connection) and OH disabling switch off the light because the "cinema" scene was enabled (via REST API).

I'm not sure I understand how this use case is related.

I meant OH switch the light OFF.

I agree and I tried to get as close as possible to that over the years. But I am not willing to do that in a way that forces the use of MQTT in a way that is against accepted best practices or in a way that limits the ability to add communicators in the future.

I fully agree with you.

Support for BTLE, Matter, or other pub/sub standards becomes very challenging to add as well if every communicator has to use the exact same cmd_src

I agree with you. So you want the sensor_reporter 'publish' interface to be open for BTLE and Matter connectors as well? With the possibility to configure an actuator to recieve messages from lets say BTLE and REST API?

What does need to be added to the Actuator and Sensor classes (possibly) is a way for the Communicator to query for it's capabilities so that it can publish the initial registration message. And that new stuff could be reused for Matter support too, which I'd love to add someday.

So sensors and actuators would get an interface to query their property e. g. datatype, measurement unit.

If it makes sense for this one openHAB communicator to do away with one or the other of cmd_src or destination, to simplify the config, I can get behind that if the changes to handle that are made to the openHAB REST communicator itself instead of the core. By making the changes to core, it makes it difficult to expand to new communicators later.

I see your point. My earlier suggestion with def publish(self, message, destination, command_src=None): would tamper with the core classes, so it's undesirable.

And I think we can get to both by adding the ability to define separate sources and destinations for each communicator.

Yes, this way there would be no limitation when combining connectors.

And maybe there needs to be an internal broker of some sort. Maybe we need to move the connection configuration away from the actuators and sensors and create a central core class where we define the wiring and the sensors and actuators just emit or receive the messages

I have a different suggestion here. What about a dictionary with "connector : topic" pairs for cmd_src and destination? E. g.

{ "mqtt" : "kitchen/main-light/cmd",
  "REST API" : "k_main-light",
  "homie" : "/homie/device1/kitchen/main-light" }

As a result the sensor/actuator config would not define the connectors in general but per "topic".

rkoshak commented 2 years ago

When you write about 'communicators' or 'connector': I assume you mean local_conn.py, mqtt_conn.py and rest_conn.py. Your mention a Homie connector: I assume you intend to leave mqtt_conn.py as it is and have new class which is mqtt_conn.py + Homie standard.

Correct on all counts. I seem to be using "connector" and "communicator" interchangeably and I don't mean too. A long time ago I built a system where the parts that connected to different protocols are called "communicators" so I seem to be falling back to old habbits.

Even though Homie uses MQTT, and I might pull some common code out into a separate parent class, they will be separate to provide flexibility for those who want to define their own MQTT topic structures or for some other reason Homie isn't suitable.

Of course Matter and BT would be wholly new things but they would be their own conn.py class too.

So you want the sensor_reporter 'publish' interface to be open for BTLE and Matter connectors as well? With the possibility to configure an actuator to recieve messages from lets say BTLE and REST API?

Yes, though I'm probably more interested in Matter than BTLE. We've already got a lot BT stuff in sensorReporter though so it seems natural to make it publish out to BT too. It always seemed like a good idea to make sensorReporter deployed to an RPi with BT into BTLE beacons for presence detection and the like.

I assume this results from my problem of connecting e. g. a actuator with two different communicators at the same time.

Yes, that's the crux of the matter. I know we'll have to support different source and destinations on the same actuator at some point so I'd prefer to just jump to that instead of jumping through hoops to make it work with openHAB and MQTT now.

I absolutely want to support both on the same actuator. But in the wider context, we really can't use the same source and destination for both and still be able to take advantage of the full capabilities of both.

With 'REST API docs' you mean using the 'API explorer' to copy/paste/edit the Generic MQTT Thing?

Yes, back in the OH 2.x days with PaperUI it was called the REST API Docs. The procedure would be to create one Generic MQTT Thing normally. Then go to the API Explorer, query for that MQTT Thing and you'll get back a JSON. Copy that JSON, edit it as needed to create the next similar MQTT Thing (e.g. change the topics) and then send that JSON back through the API to create a new one. If the edits are not too involved you can create dozens of similar MQTT Things in as little as a few minutes.

If you don't want to mess with the API Explorer, you can copy the YAML from the code tab of your first MQTT Thing, create a new one and paste that YAML into the code tab for the new Thing, editing what needs to be changed there. It's a little slower because there is more clicking involved but still pretty fast.

So you want the sensor_reporter 'publish' interface to be open for BTLE and Matter connectors as well? With the possibility to configure an actuator to recieve messages from lets say BTLE and REST API?

Yes to BTLE and Matter... eventually. sensorReporter can already receive messages from openHAB's rest API via the SSE feed. I don't know that I'd want to give sensorReporter it's own REST API though. I'd have to think on that.

So sensors and actuators would get an interface to query their property e. g. datatype, measurement unit.

Exactly. So that everything needed to discover them can be published by the Homie connector.

Though, it has been awhile since I last looked at Homie. It might be reasonable, in leu of an API on the sensors and actuators, that we encode that information in the config instead. In that case, each actuator/sensor would have a separate section for each connection it uses but that section may include more than just a source and destination depending on the information needed by Homie.

When I last started trying to implement it there was so much about how sensorReporter was architected that I couldn't even get that far along and I had to pull it completely apart and rebuild it. I had to move on to other things once I finished that so I never did get back to looking at the best way to implement Homie. But if there is enough information in the sensor/actuator to give the connection enough information to publish the Homie discovery topics and messages that would be better than requiring the end user to define them in the config.

Or maybe it needs to be a combination of the two where the config on the actuator gets passed through to the Homie connection and that becomes our API.

I have a different suggestion here. What about a dictionary with "connector : topic" pairs for cmd_src and destination?

That was my first idea too. And for now maybe that's the best approach to choose over all since it requires very little changes at all.

And of course anything added previously to handle the filter could now be backed out.

Looking ahead though, I can definitely see where we might need to provide more than just a source and destination in the config at the sensor/actuator. For a simple example, in issue #81, it would be nice to be able to indicate whether a message should be retained or not when publishing to MQTT on a sensor/actuator by sensor/actuator basis. If we have a way to do something like the following (I'll use YAML to it shows what I mean better, I'm not suggesting we must use YAML):

Connections:
    mqtt:
        source: sr/name/cmd
        destination: sr/name/state
        retained: true
    openhab:
        item: MyItem
    Homie:
        name: Foo
        # any other parameters required

then the actuator could hold on to that parsed config and pass it along with the new state in the call the publish on the connection instead of forcing each connection to conform to a cmd_src/destination scheme with no additional per connection customization possible. Each connection can look at its specific section of the config passed to it and do the right thing.

But we don't need that right now and it's kind of a big job since it impacts a lot of the config parsing (especially if we move away from .ini files). So I'd be OK with just using a dict for now. Though long term I think we'll have to move to something like this per connection config.

DanielDecker commented 2 years ago

... query for that MQTT Thing and you'll get back a JSON. Copy that JSON, edit it as needed to create the next similar MQTT Thing

Thanks for the explanation, this way the setup will be way easier. :-)

sensorReporter can already receive messages from openHAB's rest API via the SSE feed. I don't know that I'd want to give sensorReporter it's own REST API though.

I was referring to the openHAB's rest API.


The connections would need to know their own name, which I think they do, and then they can pull the source/destination from the dict based on their name. Ignore the message if it's not in the dict.

Are you referring to the reception of a message when the connector calls the subscribers. Currently any connector knows only it's own subscribers. I wouldn't change that. So for the dictionary approach I would sort the subscriptions when registering: from actuator.py

def _register(self, destination, handler):
        """Protected method that registers to the communicator to subscribe to
        destination and process incoming messages with handler.
        """
        for key, value in destination.items():
            self.connections[key].register(value, handler)

destination is a dictionary with { connector : topic } pairs.

And of course anything added previously to handle the filter could now be backed out.

Yes, indeed.

For a simple example, in issue https://github.com/rkoshak/sensorReporter/issues/81, it would be nice to be able to indicate whether a message should be retained or not when publishing to MQTT

I understand there might be none or several parameters which can modify the behavior of a connector when sending a message.

the actuator could hold on to that parsed config and pass it along with the new state in the call the publish on the connection instead of forcing each connection to conform to a cmd_src/destination scheme with no additional per connection customization possible.

Your example hints we could go even a step further and combine cmd_src and destination to one variable lets call it communication or comm for short. This variable would hold following dictionaries:

{ 
"mqtt" : { 
    "source" : "sr/name/cmd",
    "destination" : "sr/name/state",
    "retained" : true
    }
"openhan" : {
    item : "MyItem",
    }
"homie" : {
    name : "Foo" 
    }
}

Which is by the way the exact pyyaml representation of your YAML config example. Core wouldn't need to care about the contents of the sub dictionaries, it would just pass the whole sub dictionary to the connector which would have to decide where to publish and with what modifiers.

But we don't need that right now and it's kind of a big job since it impacts a lot of the config parsing (especially if we move away from .ini files).

I like the idea of using YAML for the config-file. It would make it way easier to have per connector parameters. The implementation effort depends on the way the final YAML config would look like. The following example should be a close representation of the current ini config hierarchy:

Logging:
   Syslog: YES
   Level: INFO
Connection1:
   Class: local.local_conn.LocalConnection
   Name: local
   OnEq: ON
Sensor1:
   Class: gpio.rpi_gpio.RpiGpioSensor
   Connections:
       local:
           destination: eg_k_greenLED
   Pin: 8
   PUD: UP 
   EventDetection: BOTH 
   Values:
      - OFF
      - ON

And therefore is easy to implement. If you're happy with this layout I can prepare a PR.

To sum up: I think its easier to implement pre connector topics after the config was moved to YAML syntax.


I took a closer look at the code and am now sure that the migration to a YAML configuration is easy to do. I actually finished the migration for all connections already. But in the sensors / actuators are a lot of log messages like:

self.log.info("Command results to be published to %s\n%s",
                          self.destination, output)

Since with the proposed configuration layout the actuator will only see the Connections entry. To print this whole dictionary won't make the log easier to read. So these log messages have to change too. Maybe instead of the destination we can put the section-name of the actuator in these log messages?

rkoshak commented 2 years ago

Sorry for the delay, I was on vacation.

I'm happy with that layout.

It's a big job because all the connections, actuators, sensors, and even logging expect a ConfigParser Object and has code to handle NoSuchOption exceptions. That would have to be changed and tested in the move to YAML. It's not complicated but it touches almost every file that makes up sensorReporter which is what makes it big. It's the testing that nothing is broken more so than the actual changes to the code.

Not only do those log statements need to be adjusted (I'm OK with just removing the destination from that statement entirely so it becomes self.log.info("Command results to be published:\n%s", output)). But each sensor and actuator needs to be adjusted to pull the parameters from the parsed YAML structure and handle what ever exception is thrown (if any) from that Object.

But I agree, we need to move eventually, may as well do it now.

Does it still make sense to use Sensor1 and such? That was a side effect of the fact that ConfigParser couldn't handle sections with the same name. With YAML that may not be a problem any more. It might make sense to remove that number appended to the section name. It was only ever a work around anyway.

Logging:
   Syslog: YES
   Level: INFO
Connection:
   Class: local.local_conn.LocalConnection
   Name: local
   OnEq: ON
Sensor:
   Class: gpio.rpi_gpio.RpiGpioSensor
   Connections:
       local:
           destination: eg_k_greenLED
   Pin: 8
   PUD: UP 
   EventDetection: BOTH 
   Values:
      - OFF
      - ON
DanielDecker commented 2 years ago

It's a big job because all the connections, actuators, sensors, and even logging expect a ConfigParser Object and has code to handle NoSuchOption exceptions. That would have to be changed and tested in the move to YAML. It's not complicated but it touches almost every file that makes up sensorReporter which is what makes it big. It's the testing that nothing is broken more so than the actual changes to the code.

I agree, almost every file is affected and thus need to be tested.

Does it still make sense to use Sensor1 and such?

I'm afraid we need to stick to unique "section" names, since PyYAML will import the yml-file as a dictionary into python and dictionary need unique keys. I was even thinking about to take advantage of this fact and use part of the section name as device name for the log e. g.

Logging:
   Syslog: YES
   Level: INFO
Connection1:
   Class: local.local_conn.LocalConnection
   Name: local
   OnEq: ON
SensorMainLightSwitch:
   Class: gpio.rpi_gpio.RpiGpioSensor
   Connections:
       local:
           destination: eg_k_greenLED
   Pin: 8
   PUD: UP 
   EventDetection: BOTH 

The log would use MainLightSwitch so the user knows what log entry belongs to what section: 2022-03-12 18:16:48,721 INFO - [ RpiGpioSensor] MainLightSwitch configued RpiGpioSensor: pin 8 (BCM) on destination eg_k_greenLED with PUD UP

I actually finished the migration for all connections already. But in the sensors / actuators are a lot of log messages like:

On my development branch all connectors and the exec sensors / actuators work with the YAML configuration file. It will take some time to migrate the other devices. Should I hand in a work in progress PR so you can stay up to date with the code changes?

Off topic: Since the YAML configuration file introduce breaking changes. What do you think about creating a release of the current state of sensor_reporter?

rkoshak commented 2 years ago

Should I hand in a work in progress PR so you can stay up to date with the code changes?

That would be useful. It lets me keep up to speed with the changes with smaller reviews so it's not one massive update that takes a lot of time to look at.

Off topic: Since the YAML configuration file introduce breaking changes. What do you think about creating a release of the current state of sensor_reporter?

I've never really done releases on this repo. I probably should have on the last major refactor since there were a few breaking changes there (I broke a lot of the BT stuff into separate sensors instead of having them all in one with a really complicated config).

When we get a little closer to merging this PR I'll cut a 1.0 release and make these changes a 2.0 release. Then I guess it makes sense to do point releases from that point forward.

DanielDecker commented 2 years ago

When we get a little closer to merging this PR I'll cut a 1.0 release and make these changes a 2.0 release. Then I guess it makes sense to do point releases from that point forward.

Sounds good. :-)

I had a look at all sensors and actuators to think of a unified way with the 'Connections:' parameter. So I thought again about the layout example of the RpiGpioSensor which has like most sensors several 'outputs'. Currently I prefer a solution with several 'Connections:' parameters which are grouped with the related sensor parameters. So the full example for the RpiGpioSensor would look like:

#definition of logging and connectors

SensorMainLightSwitch:
   Class: gpio.rpi_gpio.RpiGpioSensor
   Level: INFO
   Pin: 8
   PUD: UP 
   EventDetection: BOTH 
   Switch:
      Values:
         - ON
         - OFF
      Connections:
          local:
              StateDest: eg_k_greenLED
   ShortButtonPress:
      Btn_Pressed_State: LOW
      Short_Press-Threshold: 0.5
      Connections:
         mqtt:
            StateDest: main_switch/short_press
         openHAB:
            Item: eg_k_short_press
   LongButtonPress:
      Long_Press-Threshold: 2
      Connections:
         local:
            StateDest: ge_k_long_press

That way the 'Connections' dictionary will have the same layout for every sensor and a user can probably easier understand the layout than having one 'Connections' parameter with sensor related sub parameters. It is also easy to implement since the sensor doesn't need to now about the contents of the Connections dictionary. What do you think about?

rkoshak commented 2 years ago

This isn't a universal statement about this approach over all. It's specific to this example.

Wouldn't it make more sense to have a single topic and distinguish between short_press and long_press via the message/state received?

Beyond that, in general I worry about the overall complexity of this approach. That looks like a lot of book keeping and management that would need to be added to all(?) the sensors and actuators. It also pushes a lot of configuration down to the user.

On-the-other-hand it might solve a problem I had with the Govee sensor where I didn't have a good way to specify different Item names for each of the received values. It made sense for MQTT because I could just specify a root topic and it would create subtopics for temp, humidity, battery level, etc. Doing that makes less sense for OH connections and Item names though.

Does this approach work with Homie? It's been awhile since I've looked into the standard but wouldn't it want to treat SensorMainLightSwitch as one thing? If that's the case this approach wouldn't work because the connection would need to be defined at the sensor level, not at this lower level.

DanielDecker commented 2 years ago

Wouldn't it make more sense to have a single topic and distinguish between short_press and long_press via the message/state received?

I chose to have separate topics to make it easy to configure a local connection which will trigger different actuators for short and long press events.

Beyond that, in general I worry about the overall complexity of this approach. That looks like a lot of book keeping and management that would need to be added to all(?) the sensors and actuators. It also pushes a lot of configuration down to the user.

I agree with you. I thought it would be worth the effort to keep the config syntax simple, but:

Does this approach work with Homie? It's been awhile since I've looked into the standard but wouldn't it want to treat SensorMainLightSwitch as one thing? If that's the case this approach wouldn't work because the connection would need to be defined at the sensor level, not at this lower level.

Here you got me. I wasn't thinking about homie and with the proposed approach we would end with one homie thing per sensor output, witch is defiantly not the idea of homie. The opposite solution would be one 'Connections' parameter per device, which a sensor specific structure.

#definition of logging and connectors

SensorMainLightSwitch:
   Class: gpio.rpi_gpio.RpiGpioSensor
   Level: INFO
   Pin: 8
   PUD: UP 
   EventDetection: BOTH 
   Connections:
      local:
         Switch:
            StateDest: eg_k_greenLED
         LongButtonPress:
            StateDest: ge_k_long_press
      mqtt:
         ShortButtonPress:
            StateDest: main_switch/short_press
            Retain: yes
      openHAB:
         ShortButtonPress:
            Item: eg_k_short_press
      Homie:
         topic: main-light
         name: Living-room main light
         type: light
   Values:
     - ON
     - OFF
   ButtonPress:
      Btn_Pressed_State: LOW
      Short_Press-Threshold: 0.5
      Long_Press-Threshold: 2

On publish the sensor would send the message, the connections dictionary and the event name e.g. LongButtonPress to the connector. The connector would then select the relevant information and where to publish. The sensor send interface would get a new parameter (from core/sensor.py):

    def _send(self, message, comm, event_name):
        """Sends msg to the dest on all publishers."""
        for conn in comm.keys():
            self.publishers[conn].publish(message, comm[conn], event_name)
rkoshak commented 2 years ago

I think the one connection section per device is the way to go. I'm not liking the complexity of the config but don't see a way around it.

The complexity would only be there in the cases where it's needed. Those who just need the switch wouldn't have the short and long button press sections.

What about Values? That seems out of place to me. I could see where one might need different values to publish/subscribe to based on the connection type. For example, an openHAB might want ON/OFF but MQTT might need on/off or 1/0 or true/false.

I think the most complicated device will be the GPIO actuator so if we get that one right the rest should be easier.

DanielDecker commented 2 years ago

The complexity would only be there in the cases where it's needed. Those who just need the switch wouldn't have the short and long button press sections.

Sure the sections (switch, short button press, long button press) should be optional.

What about Values? That seems out of place to me. I could see where one might need different values to publish/subscribe to based on the connection type. For example, an openHAB might want ON/OFF but MQTT might need on/off or 1/0 or true/false.

I fully agree. But how to structure it? I wouldn't place it in the 'Connecitons' section since it would mix up with the connection related parameters and makes it even more complex. What do you think about connection names inside the 'Values' section?

#definition of logging and connectors

SensorMainLightSwitch:
   Class: gpio.rpi_gpio.RpiGpioSensor
   Level: INFO
   Pin: 8
   PUD: UP 
   EventDetection: BOTH 
   Connections:
      local:
         Switch:
            StateDest: eg_k_greenLED
         LongButtonPress:
            StateDest: ge_k_long_press
      openHAB:
         Switch:
            Item: eg_k_livingroom-light
   Values:
      local:
        - on
        - off
      openHAB:
        - ON
        - OFF
DanielDecker commented 2 years ago

Do you want to make bt.btscan_sensor.BtRssiSensor [DEPRECATED] compatible to YAML or just remove it?

About bt.govee_sensor.GoveeSensor. Do I understand it right: There can be several Govee devices and one GoveeSensor will publish the state for each with a different name? ~Currently with the ini config this is very easy to setup. Probably this won't work with homie. I would translate the config layout literally to YAML and leave the in depth modifications for late, when it's more clear what is needed for the homie connector.~ Is it easy to find out the address for the govee devices manually? If yes the simplest approach to fit in the goveeSensor with the YAML layout would be to gave one goveeSensor configuration for each physical sensor address. E. g.:

# logging and connections config
SensorGovieOutside:
   Class: bt.govee_sensor.GoveeSensor
   Connections:
      openHAB:
         Battery:
            Item: govieOutsd_battery
         Rssi:
            Item: govieOutsd_rssi
         Temp_c:
            Item: govieOutsd_temp_c
         Temp_f:
            Item: govieOutsd_temp_f
         Humi:
            Item: govieOutsd_humi
   Address: 11:22:33:44:55:66

The output sections (Battery, RSSI, etc.) are optional, so if someone doesn't need Temp_c he doesn't need to configure it. What do you think about that?

rkoshak commented 2 years ago

Do you want to make bt.btscan_sensor.BtRssiSensor [DEPRECATED] compatible to YAML or just remove it?

It's been deprecated for some time so let's just remove it. This will be 2.0 version of the tool with breaking changes so it's not that big of a deal to remove it I think.

About bt.govee_sensor.GoveeSensor. Do I understand it right: There can be several Govee devices and one GoveeSensor will publish the state for each with a different name?

Yes. These devices just broadcast on BTLE. The sensor picks up the Govee unique messages (among what else is being broadcast). Each one has a unique identifier (based on it's BT address I think).

Is it easy to find out the address for the govee devices manually? You need to get a message from one to know what its unique id is so there's a chicken and egg problem. You might be able to use a magnifying glass to read the small print inside the battery compartment, or use the phone app to find its UID which isn't great as a prerequisite but doable. I don't have one handy to look right now. But most BT devices I've seen put their address on the case somewhere and I think the UID is partially based on the BT address.

image

That shows a couple I've got publishing to MQTT. The GVH5072_XXXX is the reported UID for that device. I think the "XXXX" are the last two parts of the BT address.

What do you think about that?

Given the above I think that could work, assuming we have a reliable way to determine the ID.

DanielDecker commented 2 years ago

What about Values? That seems out of place to me.

How should we structure the optional Values section? Is it enough if we can specify a value pair per connection, or should a sensor like the bt.btle_sensor.BtleSensor have value pairs for each output / destination?

rkoshak commented 2 years ago

Looking through the readmes it looks like the GPIO and BtleSensor are the only two that have a Values section. For GPIO it's pretty straight forward since there can only be one per sensor.

While it would be more flexible to define the values for each output, it seems a little excessive. Would someone really want to output ON/OFF for one device's presence and OPEN/CLOSED for another one's? That seems like the sort of inconsistency we should help the users avoid. After all, in both cases all it means is that a device was detected as present or absent.

So I would say do it once for the whole sensor and let someone complain later on down the road if they need more flexibility and want to use different messages for different devices.