-
Notifications
You must be signed in to change notification settings - Fork 11
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
Fully support dual channel actors #9
Conversation
In the FHEM module, a dual actor is represented as three devices. One main device (e.g. 4376bb) and one for each channel with appended channel number (e.g. 4376bb01 and 4376bb02). For consistency, this should be also done for pyduofern, but was only partly implemented, yet. In particular, these channel-specific devices where not added as own devices and thus could not be addressed, leading to exceptions. This commit adds all three devices and also sets the state of the respective device.
Since dual actors are now represented as three devices, but two are not actually pairable devices, avoid to SetPairs for these devices.
After representing a dual actor as three devices, it is important to select the correct one when executing a command in order to select the correct chanNo. The code itself in the current implementation of the command function does not contain the channel suffix.
First of all: Thank you a lot for implementing the multi channel actor. I really appreciate it. I'm currently looking into why the changes break Travis (may take a few days). One question while I'm still at it: |
Sorry, I overlooked the CI output and did not run the unit tests. 2dc3f6a somehow leads to a blocked loop in test_replay.py. I will have a look, too, when I have the time for it. The mass replacement of code with hash has less to do with the comparison with the FHEM code, but is actually based on my assumption of the difference between Have you already had a look in the commit messages of the individual commits? They should also give some hints about the background. The main issue here is that we deal with different notions of what a "device" is. For Rademacher, a device is the physical unit that has the antenna, while for the FHEM module, a device can also be a single channel of a dual channel actor (see in particular the commit message of 2dc3f6a). In my opinion, the cleanest solution would be to stay with the Rademacher notion of a device and only (de)multiplex the device to two switches just in the home assistant component, but since this would be quite different from the FHEM approach I thought it would be better to stay with this double notion. If you have any ideas how this could be realized more smoothly and more pythonic, I am happy to change the pull request, but you can also apply them directly. Whatever you like. Also I am happy to go more into detail about my findings and can also provide real-world testing. In any case, I am very happy, that thanks to your code, I was now able to switch from FHEM to HomeAssistant for my home automation. Thanks a lot! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found out where the Travis issue comes from, the Duofern.set() method is broken (at least for rollershutters). I think I'll be able to look into this some more tomorrow.
pyduofern/duofern.py
Outdated
|
||
if ((cmd == "toggle") and (position > -1)): | ||
self.update_state(code, "moving", "moving", 1) | ||
self.update_state(hash, "moving", "moving", 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The travis timeout happens because in this line hash
is undefined (same issue with all other references to hash
in this method). To merge upstream we have to fix this.
Unfortunately just going back to using code
or device_id
here does not help because update_state
was refactored to expect the module_definition instead of a device id. It may make sense to leave this as is and instead revert to the previous behaviour of update_state
and instead adapting the code
.
pyduofern/duofern.py
Outdated
del self.modules['by_code'][code][key] | ||
def delete_state(self, hash, key): | ||
if key in hash: | ||
del hash[key] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of modifying the passed dict here, it may make sense to just fix the code
or device_id
in Duofern.parse()
. Therein one could introduce a device_id
then every time hash is set to a new value one would have to modify device_id
accordingly. Or (after reading your thoughts on device in FHEM vs device for Rademacher) one might make update_state
subdevice-aware by coming up with a a subdevice id.
pyduofern/duofern.py
Outdated
@@ -668,7 +678,7 @@ def send(self, cmd): | |||
yield from self.send_hook(cmd) | |||
|
|||
@asyncio.coroutine | |||
def set(self, code, cmd, *args): | |||
def set(self, device_id, cmd, *args): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This rename makes a lot of sense, thank you!
I pulled in the upstream modification of the unit test to stop it from entering the infinite loop. That does not fix it but at least this way we can find out faster whether a fix works. |
Good catch! Thanks! After reading your comments, I realize I was not very careful with the renaming and we should put some more thought into the names. There are at least the following names we should refine and decide if they refer to a device with or without channel: module_definition* I had a look into the Rademacher manual and they use "code" for the 6 digit identifier. And for one "code" there can be one or more "channels". So we have two options: 1.) Maintain the FHEM notion of a device and keep three devices per dual-channel actor in the 2.) Let a device refer to a device in the Rademacher sense. This would mean What is your opinion about that? |
@kaolo thank you for your clear thoughts. Firstly: At first I favored your 2 but now I think 1 may be the better way. I notice that after some thinking my considerations on suggestion 2 have become quite lengthy and 1 may be more straightforward to implement. In the context of your 2 my (not fully-thought-through) intuition about how to make the values consistent is be:
One issue I have with the explicit channel: it would add dimensions to the modules['by_code'] dict -- I find it simpler to leave that as-is and use the FHEM notion of 2(3) devices, one for each channel. (your suggestion 1) Finally: I wasn't aware that there are Rademacher manuals out that mention the code -- the one coming with my roller shutters is not explaining what happens in the background and to be frank I was already wondering how the knowledge required for the FHEM implementation was gathered. Do you have a link to a pdf version? |
The only manual I have is this one https://service.rademacher.de/hc/article_attachments/360007625334/VBD_622-1-(06.18)-DF-Universal-Aktor-9470-2-DE_Internet.pdf but even there a "Funkcode" is repeatedly mentioned. E.g. in 9.1. There is AFAIK no communication documentation and the FHEM module was built with a lot of reverse engineering skills. I also initially preferred option 2, but see that option 1 is probably faster to implement. However, maybe we should not fully give up on this yet. E.g. the dict maybe does not even need another dimension. Maybe it is sufficient to just add another parameter |
…t code not yet adapted)
@koalo I had a shot at option 2 in https://github.com/gluap/pyduofern/tree/feature/dualactor, didn't want to commit here before you can have a look. What do you think? Would this be usable for you? Caveat: I din't touch the homeassistant module yet. Was your patch already working on your end? If so it seems we can disregard if 'chanNo' in self.modules['by_code'][code]: # always false?!
chanNo = self.modules['by_code'][code]['chanNo'] But probably that was a bug I introduced while porting -- if so we could add "channel" as a keyword parameter also to Duofern.set() and use it to set the channel number. |
I this is a very good way to go forward. I made some comments in the respective commit.
I think that is a completely valid approach. It would be good if you could handle the comments I made in your commit then I can happily try to adapt the homeassistant module and test it with my setup at home.
These lines were responsible for setting the
You removed them with 9c9be2a, but as I wrote in one of the comments, using the channel parameter instead of the |
-> channel parameter is added to delete_state -> channel parameter type is modified to "int" and formatted as hex when sent to duofern.
-> replace module_definition_01 and module_definition02 consistently by channel
fix one more of those if-bugs
Thank you for your thorough review. I added the changes you proposed, I left some of the removed lines commented out and will remove them once it is tested a bit more thoroughly. edit: I added a set listing the available channels in Duofern().modules['by_code'][code]['channels'] so it is transparent to the outside which channels exist for each device. |
Pull Request Test Coverage Report for Build 55
💛 - Coveralls |
08096ed
to
f1c14c9
Compare
83fca85
to
caa1dc0
Compare
Looks very good, thanks! I made some minor modifications and updated the custom component. It now works again with my setup. I have, however, no covers, so it would be good if you test it again on your side. |
@koalo it seems my cover devices do work with the changes. Thank you for the PR as well as the updates to the custom components as well as the fixes on the way. Functionally I think we can commit this. One more request though to ensure the feature stays stable for the future: Could you set
in your/a Once this is set could you run Finally could you send me / pastebin / gist the replay files after replacing your 4 digit system code inside the files with The purpose is to cover the dual channel actor in the replay unit tests. If you are courageous you could also try to directly drop it in replaydata and run the unit tests, but that will likely break the unit tests without fiddling with the replay beforehand unless I misremember the amount of work required for the existing replays. |
I have performed the recording and pushed it to the branch, but as you expected, the test fails now. |
… to make it work (the replay mechanism gets confused if an "ack" is sent or received out of order as it expects the order to exactly match the written record)
In the FHEM module, a dual channel actor is represented as three devices.
One main device (e.g. 4376bb) and one for each channel with appended
channel number (e.g. 4376bb01 and 4376bb02).
For consistency, this should be also done for pyduofern, but
was only partly implemented, yet. In particular, these channel-specific
devices where not added as own devices and thus could not be addressed.