Draft: Support wago 750 with enocean rebase2

[toggm]

Breaking change

All changes should be backward compatible

Proposed change

This PR supports the following changes:
EnOcean:

• ESP2 Protocol support based on still open PR (EnOcean ESP2 protocol support kipe/enocean#123)
• Support of implicit enocean support based on wago modbus receiver (https://www.wago.com/global/i-o-systems/radio-receiver-enocean/p/750-642), other devices might be added later
• Support for multiple devices as Omnio RTF102, Ratio ATF101 and Omnio PM101
• Support for uniqueId and entityId to be able to modify entites in the UI as the enocean id should be unique

Modbus:

• Support for scan_groups to be able to scan all state of the same input_type at once instead of scanning them 1 by 1. Therefore with shorter scan intervals to be able to react on switch state changes fast enough to control a light
• cover:
  • Support for two-coil control of cover with up and down outputs
  • secondsToMax to be able to detect fully closed covers and calculate approximated position
• Support for wago enocean modbus receiver transfering an enocean message by reading and writing modbus registers
• Support for uniqueId and entityId based on modbus address which should be unique in a system as well

I made all the changes in one PR to be able to guarantee that all current feature of my custom home control implementation ca be covered with an adjusted home assistant system. The current implementation is now up and running for two weeks and controls all the lights, covers and reads temperature and motion detection sensors as before.
As the enocean PR is not yet merged I cannot proceed to fully finish, test and integrate the PR but would like to now if the changes applied do conform to the strategy of home assistant.

Type of change

• Dependency upgrade
• Bugfix (non-breaking change which fixes an issue)
• New integration (thank you!)
• [ x] New feature (which adds functionality to an existing integration)
• Breaking change (fix/feature causing existing functionality to break)
• Code quality improvements to existing code or addition of tests

Additional information

• Link to documentation pull request: Not yet done

Checklist

• [ x] The code change is tested and works locally.
• Local tests pass. Your PR cannot be merged unless tests pass => cannot run tests as enocean PR is not yet released
• There is no commented out code in this PR.
• I have followed the development checklist
• The code has been formatted using Black (black --fast homeassistant tests)
• Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

• Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

• The manifest file has all fields filled out correctly.
  Updated and included derived files by running: python3 -m script.hassfest.
• New or updated dependencies have been added to requirements_all.txt.
  Updated by running python3 -m script.gen_requirements_all.
• For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
• Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

• No score or internal
• 🥈 Silver
• 🥇 Gold
• 🏆 Platinum

To help with the load of incoming pull requests:

• I have reviewed two other open pull requests in this repository.

[homeassistant]

Hi @toggm,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

[probot-home-assistant]

Hey there @bdurrer, mind taking a look at this pull request as it has been labeled with an integration (enocean) you are listed as a code owner for? Thanks!
_{^{(message by CodeOwnersMention)}}

[probot-home-assistant]

Hey there @adamchengtkc, @janiversen, @vzahradnik, mind taking a look at this pull request as it has been labeled with an integration (modbus) you are listed as a code owner for? Thanks!
_{^{(message by CodeOwnersMention)}}

[toggm]

@MartinHjelmare In the modbus-enocean bridge the user could configure a local TCP port to expose and then give the user the possibility to choose between serial to tcp communicator in the config flow?

[MartinHjelmare]

Ok, but where would the bridge run? Would it be run by an integration? Which integration in that case?

[toggm]

@MartinHjelmare Yes, the bridge would be run by a customer wago modbus integration whenever enocean clamp would be activated. The wago modbus integration could then extend from the default modbus extension.

[MartinHjelmare]

Ok. The enocean code owners will need to approve the addition of the TCP connection option to the enocean integration.

[janiversen]

The group concept implemented here, do not match what the modbus protocol offers, and seems limited to sensors only. Furthermore it (mis)uses the struct something we are in the process of phasing out, especially because is has a limit of 254 chars.

The correct concept for especially coils is to do 1 read the reads many coils and then update the corresponding binary_sensors.

you also need to add tests in order to not lower the coverage score, and to make proof of your concept.

[janiversen]

you cannot use hub.name as part of the unique_id as it is something the user can change.

[janiversen]

this should be _attr

[janiversen]

Seems your code is based on an old version of the modbus integration, this code have been in modbus for quite a while.

[janiversen]

Remark my review is not complete, just to give you some items.

[toggm]

Thanks.
I did a rebase before opening the PR

[toggm]

@janiversen
The concept you described, if I understand it correctly, is exactly how I did it. I do 1 read per input_type (you cannot read discrete_inputs, coils and registers together). And then distribute the result to all registered devices (binary_sensors for discrete_input i.e.).

The current implementation is up and running against a wago modbus controller (https://www.wago.com/global/d/1256). If they stated it right in th documentation, their implementation should be manufacturer independant. But this I can't test myself.

I'm aware that tests are missing. This because at the moment I'm not able to run the tests against the modified enocean module as my PR there is not yet integrated either. I would like to work on tests as soon as this PR get's merged and you agree to be willing to integrate those conceptual changes (of course after resolving all requested changes), otherwise it doesn't make sense to invest more time.

[janiversen]

I see you are doing it for sensor, but misusing the struct parameter. But e.g. for binary_sensor and switch I cannot see you do the grouping into 1 call. Furthermore when trying your config changes in a pure modbus environment I did not manage to create a group of switches. That might of course be because you did not update the documentation.

The tests of modbus must be independent of the other integrations, so you could make them right away same goes for documentation.

Your group concept have sone advantages, but at least makes the control of the configuration a lot more difficult. One concern is that it explodes the configuration, the current discussions move more in the direction of adding the group to a single sensor and generate slave sensors:
‘’’
binary_sensors:
- name “master”
- type: coil
- address: 1
- group_count: 200
- group_names: “test1”, “test2 “ # optional.
‘’’
That will remove the need for relatively costly listeners.

As martin stated this PR is a mix of different integrations and that needs to be solved before you csn expect any indepth revviews

[toggm]

@janiversen Here we register update listeners based on a scan_group and input_type, regardless of the device itself. This has the advantage, that in the same request different devices can get updated. I created the concept of scan_group to be able to query different type of inputs with a different interval. i.e. it makes sense to listen for wall switch changes at a shorter interval than for state changes of lights.

[toggm]

@janiversen
Here we do the effectice modbus call, again independant of devices, just by input_type and per scan_group. Afterwards we update all registered devices with the starting_offset so the device knows where to start reading the results from it.
Why do you think listeners are expensive? Maybe it's just a wording issue, I meant listeners as normally used in java environments which means a reference to an object having a specific interface to be able to notify it. Is there a better way to inform a certain list of devices in pyton/HA?

[toggm]

@janiversen
You're right. The documentation is not updated yet. Same as for the tests I did wan't to get high level feedback on the changeset first. If I see that my needs are considered as an improvement to HA I'm willing to work on clean integration possible consisting of multiple merge requests. But this would require to be able to poll the modbus slave at a rate lower than one second.

An example configuration would look like:

modbus:
  - name: "wago"
    type: tcp
    host: YOUR_IP
    port: YOUR_PORT
    scan_groups:
      - name: 'switches'
        scan_interval_millis: 150
      - name: 'states'
        scan_interval_millis: 1000
    enocean:
      slave: 0
      # starting address of enocean bridge, communication happens through two input and two output registers
      input_address: 0
      # Add 0x0200 as output registers start at this address
      output_address: 512
      # Support ESP2
      esp_version: 2
    binary_sensors:
      # DI 1
      - id: "sw1"
        name: "Switch 1"
        address: 0
        input_type: discrete_input
        slave: 0
        scan_group: 'switches'
      - id: "sw2"
        name: "Switch 2"
        address: 1
        input_type: discrete_input
        slave: 0
        scan_group: 'switches'
    covers:
      - id: "sh1"
        name: "Storen Eltern"
        address: 0
        address_close: 1
        input_type: coil
        slave: 0
        scan_group: 'states'
        max_seconds_to_complete: 22
        verify:
          # offset of 0x0200 to read back value
          address: 512
          address_close: 513
    switches:
      - id: "po1"
        name: "Power outlet 1"
        address: 3
        write_type: coil
        slave: 0
        scan_group: 'states'
        device_class: outlet
        verify:
          # offset of 0x0200 to read back value
          address: 515
    lights:
      - id: "l1"
        name: "Light 1"
        address: 2        
        write_type: coil
        slave: 0
        scan_group: 'states'
        verify:
          # offset of 0x0200 to read back value
          address: 514  
enocean:
  type: 'implicit'
  device: 'modbus'  

As already mentioned to Martin, the enocean bridge could be integrated differently using a local socket and a TCPCommunicator so I could in a wago specific integration bring up a local socket and transform the request manufacturer specifc to the modbus calls and vice-versa.
But of course it would be nice to bring the non wago specific implementation parts to the base modbus integration as others might need them as well.

[janiversen]

Your example highlight some of the problems with scan_group:

• look at scan_group “states” you have grouped several coils, but how which bit in the adress represent which binary_sensor (it is very typical to have 16 coils with the same address and different bits).
• Again look at the scan_groups, you need to secure the address are identical (coils) or in sequence.
  Being in sequence is a bit of a problem, because some devices do:

    address:0 first sensor type INT32 (4 bytes)
    address: 4: second sensor ….

others do (most normal):

    address: 0 sensor FLOAT64
    address: 1 sensor …

so checking the validity of the addresses in a group and calculating how many words to read is relaying on the user configured everything correctly.

Secondly I can make a group of sensors that contain 3 sensors INT32,STRING(3),FLOAT64, this will cause an alignment problem with struct. On top of that normally STRING are variable length (count is the maximum), so there is no way of knowing where FLOAT64 starts.

These are some of the reason, why the group concept was replaced by a concept where you define a sensor/cover etc once with type, address etc, and then simply say how many you want. Pr default we then create “sensor” (master doing the actual read) and “sensor1” …. as slaves that are updated directly from the master. With that concept you are sure of the configuration.

Independently if you submit a PR to add these changes to the modbus integration, or your own variant, the problems are identical and needs to be solved.

[janiversen]

Being able to poll faster than 1 second is probably an architecture discussion, since it potentially cause changes to enter core in millisecond range, which will affect e.g. the recorder.

Forgot to mention your PR will not pass CI without tests, because modbus files are subject to 100% test coverage (controlled by coverage).

[toggm]

@janiversen I did of course not change all the scan_intervals to milliseconds and kept all changes the way they work as before without using scan_groups if wanted. Just in the configuration of scan_groups I allow scan_interval_millis.

And for the testing: As sayed I'm aware of it. Would like to now in which direction i need to modify stuff to be compatible to your architectur and then would make one or multiple adjusted PR including tests.

[toggm]

@janiversen
Internally I create a scan_group per definition AND input_type. Which means a scan_group only consists of the same type. The addresses don't have to be in sequence, in the modbus hub we query all the state for a given input_type form the minimal registered address up to the maximal registered address. There can be no duplicates of addresses in the process image per input_type...at least as far as I know.

Regarding the registers:
As far as I understood the modbus protocol reading registers consists of two values:

• starting address
• number of byte registers to read

The sensor device itself it then responsible of decoding the values out of the response based on the provided offset_address inside the full resultset. But to be honest I don't read registers from my modbus system as there are no analog input or output devices connected. If I understand you correctly, the problem might be here:

result = await self.async_pymodbus_call(
    slave, min_address, max_address + 1 - min_address, input_type
)

Where I assume that we want to read one address per listener. If that's the problem I would suggest to add a register_count to the sensor configuration and add this count to the range of addresses to read from. This would of course then affect the address_offset provided to the next device, so this one would just start reading from a further register.

Or did I miss something?

[janiversen]

Coils do have duplicates of address, because you always read a word, which is 16 coils !!

Following your assumption about addressing you are bound to run into problems, between the first address of e.g. holding, INT32, and and max. address there might be other datatypes, so you cannot simple do “read_holding” address, count = (max - first). Typically modbus device mix the datatypes (and thereby the sizes), the do NOT have all INT32 in one address set.

So your are suggesting to have “register_count” as well as “count”, that is going to very confusing to configure.

And your explanations still do not help on the string case, where e.g. I define a group of sensors:

   sensor1: address=1 count 10 type=string
   sensor2: address= 2count 5 type=string

your PR does:

    Read_holding: address=1, count=8 (remark count is always words not bytes)

the software receives:

   ABCDXYZ12

now determine which part of the string belong to which sensor ?
Whereas:

     read_holding: address=0
     read_holding: address=1

would produces results than can be added to each sensor.

As I see it, your scan_group have a problem with:

    binary_sensor -> identifying the bit within the word for a particular coil
    sensors -> input_type= STRING
    switch -> the verfify address is handled specially e.g. no scanning or with a delay after a command
    climate -> would need two scan_groups one for the active temperature and one for the set temperature, furthermore the validation needs to confirm all delay/scan are identical within a group

I did not test it, but it is interesting to see what happens if a sensor is in a scan_group but specifies a different scan_interval…..at the very least it should be reported as a configuration error.

I see a number of good enhancements in your PR, but to be honest your explanations confirm why scan_group is the more complicated solution compared to the master-slave idea.

[toggm]

@janiversen Now I'm a bit confused.
As far as I understood the specification, a coil is a single read/write bit with a specific address (https://en.wikipedia.org/wiki/Modbus#Available_function/command_codes). Over the abstraction of pymodbus requesting multiple coils in one read would mean to request the starting address as well as the number of coils to read (https://pymodbus.readthedocs.io/en/latest/source/library/pymodbus.html#pymodbus.bit_read_message.ReadCoilsResponse) and the documentation states that the result is

The coils in the response message are packed as one coil per bit of the data field

So I simply can do the following:

self._attr_is_on = bool(result.bits[address] & 1)

where the address reflects the starting address of light. The same counts for discrete_input as we only deal with a bit value.

The same counts for registers as we just define the starting address of a register and the number of registers to read, regardless of the underlying datatype (https://pymodbus.readthedocs.io/en/latest/source/library/pymodbus.html#module-pymodbus.register_read_message). So my suggestion was just to add a count reflecting the number of registers needed for a sensor to be able to determine to correct range of values to read. Looking at it more closing I saw that there is already a _count attribute which could be used for this.

How we decode the messages from the read registers afterwards should be handled in the devices itself (binary_sensor, sensor, etc). The only difference of reading it in a batch or reading it one-but-one should then be the index of the register to start reading from.

But maybe I didn't understand the documentation correctly?

At least for discrete_inputs and coils the implementation works as expected.

As for your concern of assigning a sensor to a scan_group and define a scan_interval: The first one overrule the second one, which means I only register the scan_interval in case there was no scan_group defined for this sensor:

if self._scan_interval > 0 and self._scan_group is None:
  self._cancel_timer = async_track_time_interval(
      self.hass, self.async_update, timedelta(seconds=self._scan_interval)
  )

What do you mean with:

furthermore the validation needs to confirm all delay/scan are identical within a group

Do you refer with group the type of devices?

[janiversen]

Coil is 1 bit, but not 1 but pr address. Doing read_coil address 17, count 2, returns 16bit coil1 is bit 0 and coil 2 is bit 1
Doing read_coil address 19 count 18, returns 32bit
please observe that we use read_coil not read_bit
etc. But you must have seen this already in your tests.

My concern about the scan_interval is that the user should be informed.

[toggm]

@janiversen
Just came to my mind that my assumption taking count into the queries range of addresses war wrong. I just would have to add the max of all addresses+ their counts count as the max of the address range to query to allow overlapping reads.

[toggm]

@janiversen

Coil is 1 bit, but not 1 but pr address. Doing read_coil address 17, count 2, returns 16bit coil1 is bit 0 and coil 2 is bit 1
Doing read_coil address 19 count 18, returns 32bit

Yes, that's true. But I read the results in the binary_sensor then by reading the correct bit of the provided offset-address. Means coil1 reads bit[0], coil2 reads bit[1]. As sayed, coils and discrete_inputs are working fine. t the moment I cannot proof that reading registers does work as I don't have such a device and I'm not yet able to run the tests. But I will keep an eye on this.

[janiversen]

I think discussing further in this PR does not lead to a better PR. The PR still include 2 integrations which are not allowed. I have tried to help make it a workable modbus solution by e.g. pointing at the problems in the current PR., but you have until now not make a single code change.

Your scan_group is not a bad idea, buy you need to address the obvious problems and submit a PR with changes to the modbus integration only or follow the suggestions made in this PR. When I see such a PR I will do a proper review.

[toggm]

i didn't do a code change yet because of two reasons:

1. As mentioned I would like to split the current PR into separate once as soon as I'm aware of what kind of work would be accepted
2. I'm still struggling with the decision to do this work as I felt your attitude very skeptical and want to avoid doing the work for nothing. As an alternative I could invest this time be keeping my custom implementation up-to date as I have a running system working fine and covering all my needs. Of course this would not help the community...

To summarize I suggest to split this PR into multiple once with the following featureset:

• Create a PR to support TCPCommunicator and adjust the config-flow
• Create a PR in the enocean module to support ESP2 as soon as the enocean library with that support gets released
• Create a PR in the modbus module with all the modbus changes except the wago specific enocean integration (including tests for scan_interval and the other changes)
• Create a wago-modbus integration extending the modus module with support for enocean over modbus through a local socket

@janiversen Maybe you can, as a last effort, agree with the above mentioned steps. After this I can close this one and will refer to it as soon as created new once.
Anyway thanks for your time.

[janiversen]

I cannot help you with your decision, I have outlined problems and potential solutions, but the solution is yours to make.

In principle your steps sound logical. It is true I am skeptical mainly because I have shown you the problems but not received ideas/feedback on how to solve them.

It is a feature eagerly waited for, so I am both open and positive, however I must ensure it works in all scenarios to avoid problems later.

[toggm]

@janiversen I always asked back because I didn't and stil don't understand the mentioned adressing problem. But I will try to extract only the modbus changes without enocean support into a different branch and adapt the existing tests to test as well the same behaviour with scan_groups. So either this will help me to understand the problem or I can make another PR based on this.

[janiversen]

If you have solved the other problems you are far, I look forward to see how you solve the string problem among others.

[github-actions]

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.