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

EnOcean ESP2 protocol support #123

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

toggm
Copy link

@toggm toggm commented Aug 10, 2021

After reading discussion in #72 I followed a different approach to integrate enocean esp2 protocol messages.
Based on the work of stoney, re-used from the open PR https://github.com/kipe/enocean/pull/78/files#diff-bcf3908433bf40bcb17727cba4377f97e937a7bbddea6d871d3de521b28607cc

As I don't have an standalone enocean controller I'm not able to test the implementation. Would be great if someone with a controller could do the testing and provide me protocol examples so I could extend the PR with test cases.

@kipe
Copy link
Owner

kipe commented Aug 12, 2021

In general this looks like a good base, the only big issue for me is that there's no backwards compatibility.
I'd like to have the Packet remain the same, maybe introducing BasePacket as the common base. Otherwise there's a lot of work to be done with custom communicators, which I know many projects use.

Also the common base should define parse_msg, create etc, just raising a NotImplementedError.

This would be a great addition, as the Eltako devices seem very good. Another thing to consider is that can we just leave out passing the version to Communicator and try parsing both versions. With the current implementation it wouldn't be possible to use both protocol versions at the same time.

Of course hardware testing is required, are there any volunteers for that?

@toggm
Copy link
Author

toggm commented Aug 12, 2021

@kipe Thanks for the review. I will adjust the suggested change with the Packet to remain backward compatible. For the idea to resolve the protocol version dynamically:
I first thought about this as well but decided for the configuration way as normally an enocean controller supports either ESP2 or ESP3 and not both of them. Right?

Yesterday I tried to test the ESP2 protocol implementation with my hardware. But as I'm using a Wago 750-650 controller which integrated enocean over modbus I had to do some adjustments within hass as well.

Would be great if someone having a Eltako device could test the implementation as well.

@toggm
Copy link
Author

toggm commented Aug 12, 2021

@kipe I just rethought your suggested solution and see a different problem with it. I.e. in the hass enocean integration a type check to RadioPacket exists. If I now would adjust the naming the default names reflect ESP3 protocol and we move base logic to i.e. a BaseRadioPacket, those instance checks wouldn't be compatible to ESP2 either.
Therefore I suggest the following workaround:
I keep the separate naming for ESP2 and ESP3 but implement all the static methods in the base implementations as well, forwarding the request to the ESP3 based implementations. This was all type checks are still compatible and support ESP2 out-of-the-box but without a change the system would just create ESP3 protocol packages.

@kipe
Copy link
Owner

kipe commented Aug 13, 2021

@toggm Yeah, the target should be that no changes are required for ESP3. Changes for ESP2 support on programs using this library would be fine.

In practice this is shown (imo), if no changes are requirered to the tests or the current examples.

@toggm
Copy link
Author

toggm commented Aug 17, 2021

@kipe I've now adjusted the implementation, you might have a look again. Reverted changes in tests and examples to stay backward compatible.
Furthermore I tested the ESP2 protocol with my customer enocean-over-modbus implementation in hass for Omnio RTF102 and Ratio ATF101 devices which worked as expected.

@toggm
Copy link
Author

toggm commented Sep 1, 2021

@Stoney49th can you maybe do a hardware testing?

@Stoney49th
Copy link

Stoney49th commented Sep 3, 2021

@toggm @kipe
Sure, my current setup is a tcp-serial device server, connected to a larger scale eltako installation (~30 devices, on the RS485 bus and wireless). Connection is via RS232 to an eltako RS232 bridge device. The USB-ones were a bit out of scope, since my server rack is a bit far away from my electrical cabinet.

What would be required for the test? I'm pretty busy at the moment, but maybe I can do it on one of the coming weekends...

Edit:
I'm also not using a direct integration into home assistant, but used MQTT as an intermediate protocol...much easier and fits the general abstraction model in my home better than a "deep integration"

@toggm
Copy link
Author

toggm commented Sep 6, 2021

@Stoney49th That would be great!

@kipe What do you think, what kind of hardware tests are sufficient to proof the implementation works for most of the devices?

@toggm
Copy link
Author

toggm commented Oct 14, 2021

Any progress on this?

@vincentdieltiens
Copy link

I'm also interested in ESP2 support to control Eltako 14 controllers.

I will test this evening if I can make it work in home assistant and my custom component.

I see however a limitation : the communicator can only handle one type of protocol at a time : either ESP2 or ESP3 as it is a parameter of the communicator constructor.
Is there any way to distinguish an ESP2 packet from an ESP3 one ?

@toggm
Copy link
Author

toggm commented Oct 26, 2022

I'm also interested in ESP2 support to control Eltako 14 controllers.

I will test this evening if I can make it work in home assistant and my custom component.

I see however a limitation : the communicator can only handle one type of protocol at a time : either ESP2 or ESP3 as it is a parameter of the communicator constructor. Is there any way to distinguish an ESP2 packet from an ESP3 one ?

Hi @vincentdieltiens
Yes, the main difference between ESP2 and ESP3 ist the paket size. I implemented such a check in the home-assistant integration (https://github.com/home-assistant/core/pull/58916/files#diff-308c57e05884293d792f80c532cc048b9662da4e23d41bec09ebceb87bf68243R54)

Feel free to further improve this implementation.

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.

4 participants