Refactor Telegram.payload to support other APCI payloads
#519
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I have two MyPy errors left, which I don't understand: False positive? |
|
Have you seen this: python/mypy#7423 |
|
when I run this now from my HA test installation I get a lot of 2020-12-01 22:12:26 WARNING (MainThread) [xknx.log] 'from_knx()' not implemented for GroupValueRead
# and this for GAs in fire_event_filter
2020-12-01 22:12:26 WARNING (Recorder) [homeassistant.components.recorder] Event is not JSON serializable: <Event knx_event[L]: data=<GroupValueResponse dpt="<DPTArray value="[0x19,0x62]" />" />, destination=5/1/0, direction=Incoming, source=1.0.54, telegramtype=GroupValueResponse>also I think it would be cleaner if the APCI services could get their own module for importing them instead of importing from xknx.telegram together with higher-order classes # eg
from xknx.telegram import GroupAddress, IndividualAddress, Telegram
from xknx.telegram.apci import GroupValueRead, GroupValueWrite
# instead of
from xknx.telegram import GroupAddress, GroupValueRead, GroupValueWrite, IndividualAddress, Telegram |
|
@farmio Thanks for testing, I'll pick it up. I'm also busy with a few other improvements. |
farmio
reviewed
Dec 1, 2020
|
I'm still working on the APCI unit tests. |
|
Unit tests are ready. Let's see what I have to add more :-) |
basilfx
force-pushed
the
feature/apci_payload
branch
4 times, most recently
from
6445ed1 to
b3196da
Compare
farmio
reviewed
Dec 8, 2020
basilfx
force-pushed
the
feature/apci_payload
branch
from
0e2c435 to
899b441
Compare
This becomes unnecessary if the telegram type can be derrived from the payload.
Thi adds payload classes that represent APCI messages, such as GroupValueRead/Write/Response.
This adapts all Telegram payload objects to the corresponding APCI payload objects. It removes the need for `Telegram.telegramtype`.
This can be derived from the payload of the telegram.
Byte 8 (mpdu_len) is the length of the APDU minus one. This can be calculated from payload.calculated_length().
Unless a developer uses the TelegramQueue or Device class directly, there doesn't seem to be a need for this check.
This removes the need for a constructor and false implementations of the abstract methods.
Typing was necessary here for APCI code to be type checked.
basilfx
force-pushed
the
feature/apci_payload
branch
from
899b441 to
df2fccd
Compare
farmio
approved these changes
Dec 8, 2020
farmio
reviewed
Dec 9, 2020
|
Thanks! |
|
Thank you! I think its a great addition. 🙏 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This is the second part of #253, where
Telegram.payloadis refactored to support generic APCI payloads. In addition,Telegram.telegramtypeandCEMIFrame.cmdare obsolete as well, because they can be derived from the payload.Most of the changes are related to the testcases, where
Telegram(.., payload=DPT*())is now (for example)Telegram(.., payload=GroupValueWrite(DPT*())).I have not tested the Home-Assistant plugin, as I don't have a setup running. It probably needs some changes.
Type of change
Checklist: