sensorReporter icon indicating copy to clipboard operation
sensorReporter copied to clipboard

homie connector for sensor_reporter

Open DanielDecker opened this issue 3 years ago • 4 comments

This draft PR implements the homie protocol with a new connector. Moreover this fixes #100, I hope you don't mind that I put this fix in this big PR. What does this PR change in detail:

  • fixed GpioSensor Parameter PUD and Btn_Pressed_State not optional, fixes #100
  • fixed some misleading comments so we can close #95 finally (the actual problem was already fixed with #97 )
  • modified MQTT connector to use it as parent for the new added homie connector, fixes #68
  • added interface so sensors / actuators can provide information needed for auto discovery (e. g. name and datatype of sensor outputs)
  • modified most of the sensors / actuators to use the above mentioned interface

TODO:

  • [x] ~implement auto discovery interface for RokuAddressSensor (see question below)~ marked Roku as DEPRECATED
  • [x] ~does the local.local_logic.LogicOr need the auto discovery interface?~ localOr marked as not supported

I didn't implemented the auto discovery interface for RokuAddressSensor yet. Currently the RokuAddressSensor doesn't know the Roku devices name at startup it will receive them only after discovering the Roku device. To make homie sensor auto discovery work best all the sensor outputs should be known at startup. There are several solutions to this:

  1. Add a new Parameter to the Roku sensor which lists the expected Roku device names. This would break old configuration even if the homie connector isn't used
  2. Let the user add the necessary entries (output name, datatyp etc.) in the RokuSensor > Connection > Homie section manually to make auto discover work. I believe we discussed this already in #68 and decided against it
  3. Introduce a new parameter for the homie connection which lists all Roku device names in the network e. g.
Connection:
   Homie:
      Name: MySensorName
      KnownOutputs:
         - Roku remote
         - Roku TV

Currently I prefer solution 3. What do you think about it?

Moreover I didn't implemented the auto discovery interface for local.local_logic.LogicOr since it is mainly targeted for local connections and doesn't fit with the current architecture of the homie connector. Which supports sensors with several outputs and actuators with one input and one output. Btw. the LogicOr has several inputs and one output. I'd like to leave the LogicOr without auto discovery support for now. Is this OK with you?

DanielDecker avatar Oct 31 '22 12:10 DanielDecker

This draft PR implements the homie protocol with a new connector. Moreover this fixes https://github.com/rkoshak/sensorReporter/issues/100, I hope you don't mind that I put this fix in this big PR.

I would prefer they were done separately so if this big PR runs into some issue or otherwise takes a long time the fix to that other issue doesn't have to wait. But I'll not have you back it out now. It's also easier to review if the PR is focused on only one issue at a time.

If it looks like this is going to take too long, we can extract just that fix and submit it as a separate PR later if we need to.

Currently I prefer solution 3. What do you think about it?

I agree, 3 is probably the best and it's consistent with how we discover the BT addresses for Govee.

Or we can just decide to deprecate Roku. OH now has a good Roku add-on so the need for this isn't that great any more. I suspect on the off chance that users of other automation systems are using sensorReporter I bet they too have native support for Roku by this point too.

If Roku is the only problem child, I'd be inclined to deprecate it and not do too much to make it support Homie. On-the-other-haand, I can see this chicken-and-egg problem happening in other stuff in the future so maybe it's worth figuing out now just to have it in our back pocket when we really need it.

I wonder if another option is to do an initial publication to an MQTT topic with discovered stuff and then use that on the next restart to fill out the rest of the Homie info. This idea isn't even half baked, just brain storming.

the LogicOr has several inputs and one output. I'd like to leave the LogicOr without auto discovery support for now. Is this OK with you?

I think so. It's local after all so I don't think it makes sense to expose it to Homie or anything else external anyway.

rkoshak avatar Oct 31 '22 17:10 rkoshak

If it looks like this is going to take too long, we can extract just that fix and submit it as a separate PR later if we need to.

I see. So I'll finish this PR fast ;-)

On-the-other-haand, I can see this chicken-and-egg problem happening in other stuff in the future so maybe it's worth figuing out now just to have it in our back pocket when we really need it.

The homie spec recommends in such a case to set the communication status back to "initializing", publish the new sensor channels and set the status back to "ready". But this results in changing availability of sensor channels visible to OH depending if sensor_reporter just has started or is running several hours. This could be confusing, there for I'd like to avoid it.

We could mark the Roku sensor in the documentation as deprecated and leave it simply unsupported for the homie connector. Would this be OK with you?

I think so. It's local after all so I don't think it makes sense to expose it to Homie

Ok, I'll write it down in the documentation accordingly.

DanielDecker avatar Oct 31 '22 21:10 DanielDecker

But this results in changing availability of sensor channels visible to OH depending if sensor_reporter just has started or is running several hours. This could be confusing, there for I'd like to avoid it.

But if this is the standard documented way that Homie is supposed to work, if it's confusing on the OH side of things then it's an OH problem and it should be fixed there. I don't like the idea of deviating from the standard unless we really need to. And I know a lot of work has been done on Homie in openHAB, maybe it can handle this already gracefully?

We could mark the Roku sensor in the documentation as deprecated and leave it simply unsupported for the homie connector. Would this be OK with you?

I'm OK with that too. Everything seems to support Roku natively now so I don't see it being worth keeping around just in case. If someone complains we can readdress later.

rkoshak avatar Nov 01 '22 14:11 rkoshak

The homie spec recommends in such a case to set the communication status back to "initializing", publish the new sensor channels and set the status back to "ready".

But if this is the standard documented way that Homie is supposed to work

This is optional for devices which change there in-/ outputs at runtime. So it's up the the developer of the device if he wants to change these at runtime. We can continue this discussion in a new issue if you like. For all current sensors and actuators (except Roku) the in- /outputs are known at startup, so there is no problem.

With my last commit I changed the readmes for the Roku and the localOr sensor as we discussed (marked as incompatible with homie). So from my point of view this PR is ready for merge.

DanielDecker avatar Nov 03 '22 16:11 DanielDecker

@rkoshak This pull request has been ready for merge since 3 November. Now it shows a conflict with 2 files. I'm not sure how to proceed. Are you still interested in Homie support via MQTT?

DanielDecker avatar Jan 29 '23 14:01 DanielDecker

I totally missed that it was ready for merge. I would have done so if I realized. I thought it was still in work.

I am still interested in Homie.

I think there was only one PR since November.

I'll need to get on a computer to see what's going on which won't happen until tomorrow, this evening at the earliest.

I suspect we can just pull in the change it's complaining about to this PR but that sort of thing is completely new to me so I'll have to look up how to do it.

rkoshak avatar Jan 29 '23 17:01 rkoshak

Thanks for your replay. Solving merge conflicts is also new to me. Tell me if it's easier to resolve the conflicts on my end! :-)

DanielDecker avatar Jan 30 '23 16:01 DanielDecker

Ok, I merged the changes from your main branch to this pull request using github's conflict editor (via resolve conflict button). So this PR is ready for merge! =)

Obviously the new gpio.ds18x20_sensor.Ds18x20Sensor need some small code changes to work with Home. I'll solve this with a new PR.

DanielDecker avatar Jan 30 '23 17:01 DanielDecker