Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix telegram for switching off in light.py #135410

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from

Conversation

jotzt
Copy link

@jotzt jotzt commented Jan 11, 2025

Proposed change

For switching off the dimmer like Eltako FUD61… or FD62…, the data byte 0 bit 0 should be 0. Accordingly, change the last byte in the “switch off” telegram to 0x08.

The fix changes the EnOcean telegram payload only. It has been tested successfully using an Eltako FD62NPN-230V dimmer.

Further information is available in the Eltako reference document https://www.eltako.com/fileadmin/downloads/en/_main_catalogue/Gesamt-Katalog_ChT_gb_highRes.pdf

Type of change

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

Additional information

None

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Ruff (ruff format homeassistant tests)
  • Tests have been added to verify that the new code works.

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

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.

To help with the load of incoming pull requests:

For switching of the dimmer like Eltako FUD61… or FD62…, the data byte 0 bit 0  should be 0. Accordingly, change the last byte in the telegram to 0x08.

Further information is available in the Eltako reference document https://www.eltako.com/fileadmin/downloads/en/_main_catalogue/Gesamt-Katalog_ChT_gb_highRes.pdf
Copy link

@home-assistant home-assistant bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jotzt

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!

@home-assistant
Copy link

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@home-assistant
Copy link

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!

Code owner commands

Code owners of enocean can trigger bot actions by commenting:

  • @home-assistant close Closes the pull request.
  • @home-assistant rename Awesome new title Renames the pull request.
  • @home-assistant reopen Reopen the pull request.
  • @home-assistant unassign enocean Removes the current integration label and assignees on the pull request, add the integration domain after the command.
  • @home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
  • @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

@jotzt jotzt marked this pull request as ready for review January 12, 2025 09:09
@joostlek
Copy link
Member

I personally have no clue if this PR is good or not. So I would like some more eyes from people who know about enocean. @andreas-bulling, can you maybe shine a light as you also have a PR open for this integration?

@jotzt
Copy link
Author

jotzt commented Jan 14, 2025

The fix of this PR is included in PR #134594
If the PR 134594 is merged, this PR will be rendered obsolete. On the other hand, this PR fixes only one bit, maybe it is easier to merge this first if the other PR requires more attention. There will be no issue if this PR 135410 is merged before PR 134594.

@joostlek
Copy link
Member

that PR won't be merged as is, as it changes the configuration structure.

So in order for me to know if this PR is good, I think I want to know if this work with all the devices this integration supports?

Because I have a hard time thinking that this is broken since the start and never caused a problem for anyone, and I don't feel like fixing it for a few people to break it for the others. :)

@jotzt
Copy link
Author

jotzt commented Jan 14, 2025

The Eltako documentation tells the same for the FUD61 type (referred as working with HASS EnOcean) and FD62 typ (not working w/o fix):

DB0_Bit0 = 1: Dimmer ON, 0: Dimmer OFF.

Maybe the FUD61 type still turns off as the dimming value in the existing telegram is set to 0:

command = [0xA5, 0x02, 0x00, 0x01, 0x09]

The third hex block (Data byte 2) contains the dimming value (00…FF), the last hex block (Data byte 0) contains the telegram type on bit 3 (1 = data telegram) an the switching command on bit 0 (see above). According to the Eltako documentation, the last bit has to be 0 (0x08) for switching off. This applies to all Eltako devices.

@andreas-bulling
Copy link

I am afraid I am by no means an EnOcean expert. I found a working solution in #134594 by looking at the official documentation as well as trial and error.

As you can see in the code that I proposed, I am using the "old" way in which a RadioPacket is created by setting individual bits. To switch off, I am also setting SW=0 , which probably corresponds to the bit that @jotzt modified

@jotzt
Copy link
Author

jotzt commented Jan 14, 2025

That is the case, from encocean/protocol/EEP.xml

    <profiles func="0x38" description="Central Command">
      <profile type="0x08" description="Gateway">
[…]
          <enum description="Switching command ON/OFF" shortcut="SW" offset="31" size="1">
            <item description="Off" value="0" />
            <item description="On" value="1" />

SW defines the last bit of the 4BS which matches flawlessly with the previous explanation.

The most valid testing I and possibly @andreas-bulling could provide are the actually generated EnOcean telegrams.

Regarding this PR, the telegrams are as follows:
Switching on (minimum brightness), dimming, switching off

2025-01-14 20:16:55.910 DEBUG (Thread-2) [enocean.communicators.SerialCommunicator] 0x01 ['0xa5', '0x2', '0x1', '0x1', '0x9', [4 byte of destination id], '0x0'] [] OrderedDict()

2025-01-14 20:19:03.137 DEBUG (Thread-2) [enocean.communicators.SerialCommunicator] 0x01 ['0xa5', '0x2', '0x43', '0x1', '0x9', [4 byte of destination id], '0x0'] [] OrderedDict()

2025-01-14 20:21:46.512 DEBUG (Thread-2) [enocean.communicators.SerialCommunicator] 0x01 ['0xa5', '0x2', '0x0', '0x1', '0x8', [4 byte of destination id], '0x0'] [] OrderedDict()

Conclusion: All packets are well-formed and working.

@jotzt
Copy link
Author

jotzt commented Jan 14, 2025

I am using the "old" way in which a RadioPacket is created by setting individual bits

Is there a more advanced approach? Using the packet generation from Python EnOcean – from my point of view – is clearly an improvement.

[Edit] Read your comment regarding the other PR. I am going to move the discussion to discord as offered by joostlek.

@andreas-bulling
Copy link

andreas-bulling commented Jan 14, 2025

It seems the EnOcean integration was updated at some point. I do this

packet = RadioPacket.create(
    rorg=RORG.BS4,
    rorg_func=0x38,
    rorg_type=0x08,
    destination=self._destination_id,
    sender=self._sender_id,
    COM=1,
    command=1,
    DEL=0,
    LCK=0,
    SW=1)

self.send_packet(packet)

The newest version of the integration code uses something like

command = [0xA5, 0x02, bval, 0x01, 0x09]
command.extend(self._sender_id)
command.extend([0x00])
self.send_command(command, [], 0x01)

@joostlek
Copy link
Member

@andreas-bulling if you feel like it, you can find a chat about at discord at. https://discord.com/channels/330944238910963714/1328816932408922142. Be sure to select the developer role when joining the server

@jotzt
Copy link
Author

jotzt commented Jan 17, 2025

This single bit is definitely broken in the current version. I had to fix it again in my local installation to get the device working after the update to 2025.1. Considering that the PR is also compliant with the manufacturer documentation, there is no reason from my point of view why the fix could break anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants