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

Wrong structure of DPT 251.600 payload in remote_value_color_rgbw.py #229

Closed
dstrigl opened this issue Aug 27, 2019 · 28 comments
Labels

Comments

@dstrigl
Copy link
Contributor

@dstrigl dstrigl commented Aug 27, 2019

The structure of the DPT 251.600 payload in file remote_value_color_rgbw.py is wrong.

It should be:

Structure of DPT 251.600

  • Byte 0: R value
  • Byte 1: G value
  • Byte 2: B value
  • Byte 3: W value
  • Byte 4: 0x00 (reserved)
  • Byte 5:
    • Bit 0: W value valid?
    • Bit 1: B value valid?
    • Bit 2: G value valid?
    • Bit 3: R value valid?
    • Bit 4-7: 0

The bitfield which defines whether an value (R,G,B,W) is valid or not should be in the last byte of the array and not at the beginning. This can be verified by trying to send a DPT 251.600 telegram in the BUS monitor of the ETS5 application.

In the following I already added a fix for this issue (function to_knx() and from_knx() in remote_value_color_rgbw.py):

    def to_knx(self, value):
        """
        Convert value (4-6 bytes) to payload (6 bytes).

        * Structure of DPT 251.600
        ** Byte 0: R value
        ** Byte 1: G value
        ** Byte 2: B value
        ** Byte 3: W value
        ** Byte 4: 0x00 (reserved)
        ** Byte 5:
        *** Bit 0: W value valid?
        *** Bit 1: B value valid?
        *** Bit 2: G value valid?
        *** Bit 3: R value valid?
        *** Bit 4-7: 0

        In case we receive
        * > 6 bytes: error
        * 6 bytes: all bytes are passed through
        * 5 bytes: 0x0f right padding
        * 4 bytes: 0x000f right padding
        * < 4 bytes: error
        """
        if not isinstance(value, (list, tuple)):
            raise ConversionError("Cannot serialize RemoteValueColorRGBW (wrong type, expecting list of 4-6 bytes))",
                                  value=value, type=type(value))
        if len(value) < 4 or len(value) > 6:
            raise ConversionError("Cannot serialize value to DPT 251.600 (wrong length, expecting list of 4-6 bytes)",
                                  value=value, type=type(value))
        rgbw = value[:4]
        if any(not isinstance(color, int) for color in rgbw) \
                or any(color < 0 for color in rgbw) \
                or any(color > 255 for color in rgbw):
            raise ConversionError("Cannot serialize DPT 251.600 (wrong RGBW values)", value=value)
        return DPTArray(list(value) + [0x00, 0x0f][len(value)-4:])

    def from_knx(self, payload):
        """
        Convert current payload to value. Always 4 byte (RGBW).

        If one element is invalid, use the previous value. All previous element
        values are initialized to 0.
        """
        result = []
        for i in range(0, len(payload.value) - 2):
            valid = (payload.value[5] & (0x08 >> i)) != 0  # R,G,B,W value valid?
            result.append(payload.value[i] if valid else self.previous_value[i])
        self.previous_value = result
        return result
@Julius2342

This comment has been minimized.

Copy link
Collaborator

@Julius2342 Julius2342 commented Aug 27, 2019

@phbaer : May you have a look?

@phbaer

This comment has been minimized.

Copy link
Contributor

@phbaer phbaer commented Aug 27, 2019

I followed the format description found in https://www.mdt.de/download/MDT_THB_DaliControl_IP64_Gateway_03.pdf which defines the format of DPT 251.600 as: (r_12, B_4, U_8, U_8, U_8, U_8). This is why I assumed the bit order should be as implemented. And at least my controller (MDT Dali GW) seemed to work as expected.
Maybe that assumption was wrong... ETS should know how a packet should look like. Is there an official DTP format definition somewhere?

@dstrigl

This comment has been minimized.

Copy link
Contributor Author

@dstrigl dstrigl commented Aug 27, 2019

Hi @phbaer,

I tested it with a MDT AKD-0424V.02 LED Controller connected to a RGBW stripe.

Here are two screenshots from the ETS5 bus monitor:

2019-08-27 20_27_52-ETS5™ - KNX-Testbrett

2019-08-27 20_29_40-ETS5™ - KNX-Testbrett

The used GA to set the RGBW values with DPT 251.600 is 1/1/40, the status response is on GA 1/1/41. As you can see, the values for R, G, B and W (100% = 0x64, 80% = 0x50, 60% = 0x3C and 40% = 0x28) are in the first four bytes, followed by an reserved 0x00 and the bitfield with the definition of valid values (0x0f).

So I think your assumption was wrong ...

Regards,
Daniel.

@phbaer

This comment has been minimized.

Copy link
Contributor

@phbaer phbaer commented Aug 27, 2019

Hi @dstrigl,

ok, sounds reasonable. I'll check when I'm back home. Could well be that I did nix up something. Could you prepare a pull request that I could quickly test? That would be great :)

Thanks!

@dstrigl

This comment has been minimized.

Copy link
Contributor Author

@dstrigl dstrigl commented Aug 27, 2019

Hi @phbaer,

Could you prepare a pull request that I could quickly test? That would be great :)

I will try to prepare it tomorrow ... and test it ...

Regards,
Daniel.

@marvin-w

This comment has been minimized.

Copy link
Collaborator

@marvin-w marvin-w commented Aug 27, 2019

Hi there,

I found another source here: https://www.theben.de/ocsmedia/optimized/full/o5641v42%20KNX-DALI%20Gateway%20plus%20-%20Hand%20book.PDF

It also states that the current implementation is correct.

There is an official KNX specification that can be downloaded from your MyKNX => Downloads for free. (03_07_02_Datapoint Types*.pdf)

However, the current version doesn't contain this DPT.

@dstrigl

This comment has been minimized.

Copy link
Contributor Author

@dstrigl dstrigl commented Aug 27, 2019

Hi @marvin-w,

I found another source here: https://www.theben.de/ocsmedia/optimized
/full/o5641v42%20KNX-DALI%20Gateway%20plus%20-%20Hand%20book.PDF

It also states that the current implementation is correct.

that's not really true. The text in the given Theben document (written in German):
"... In den unteren Bytes werden die Farbwerte für Weiß, Blau, Grün und Rot im Wertebereich von 0..100% angegeben. Im 5. Byte geben 4 Bits an, ob die entsprechenden Farbwerte gültig sind."
means that in the lower 4 bytes are the values for white, blue, green and red and in the fifth byte the 4 bits define whether the values are valid or not.

@marvin-w

This comment has been minimized.

Copy link
Collaborator

@marvin-w marvin-w commented Aug 27, 2019

Well, the same thing is stated in the other source, apparently.

Or am I wrong?

"
Über dieses Objekt kann die Farbe als RGBW in der Gruppe eingestellt werden. In den unteren Bytes
werden die Farbwerte für Weiß, Blau, Grün und Rot im Wertebereich von 0..100% angegeben. Im 5.
Byte geben 4 Bits an, ob die entsprechenden Farbwerte gültig sind.
"
https://www.mdt.de/download/MDT_THB_DaliControl_IP64_Gateway_03.pdf

for me they both look equal.

And the format is still (r_12, B_4, U_8, U_8, U_8, U_8).

@dstrigl

This comment has been minimized.

Copy link
Contributor Author

@dstrigl dstrigl commented Aug 27, 2019

And the format is still (r_12, B_4, U_8, U_8, U_8, U_8).

But the ETS5 bus monitor is sending a:
U_8, U_8, U_8, U_8, r_12, B_4
for the DPT 251.600, as you can see in the upper screenshots. Or I am wrong?

@phbaer

This comment has been minimized.

Copy link
Contributor

@phbaer phbaer commented Aug 27, 2019

Are the KNX devices big or little endian? Confusing that would explain the issue.

@phbaer

This comment has been minimized.

Copy link
Contributor

@phbaer phbaer commented Aug 27, 2019

I'll check with my Dali GW and the ETS when I'm back. Should be fairly easy. Than you, @dstrigl, for raising that and testing with a different device! A PR would be very nice still :D

@farmio

This comment has been minimized.

Copy link
Collaborator

@farmio farmio commented Aug 27, 2019

@dstrigl have you had issues with an actual device with the current implementation?

Endianness of the devices processor wouldn't affect the protocol.

@phbaer

This comment has been minimized.

Copy link
Contributor

@phbaer phbaer commented Aug 27, 2019

The byte order of the message, as shown by the ETS, is reverse to what I implemented. So not the byte order of the device, that's true.

@dstrigl

This comment has been minimized.

Copy link
Contributor Author

@dstrigl dstrigl commented Aug 27, 2019

@dstrigl have you had issues with an actual device with the current implementation?

Yes, with the called MDT LED Controller in the home assistant application (which uses the xknx implementation).

@farmio

This comment has been minimized.

Copy link
Collaborator

@farmio farmio commented Aug 27, 2019

Oh sorry I must have read over your post.
So @phbaer uses a device of the same manufacturer and still needs a different implementation.
It's really time for KNX association to update their specifications...

@phbaer

This comment has been minimized.

@dstrigl

This comment has been minimized.

Copy link
Contributor Author

@dstrigl dstrigl commented Aug 28, 2019

Pull request created, see: https://github.com/XKNX/xknx/pull/231

I tested it again with my MDT AKD-0424V.02 LED controller using the added example_light_rgbw.py in the PR. It works fine with the changes in the PR ...

Regards,
Daniel.

@marvin-w

This comment has been minimized.

Copy link
Collaborator

@marvin-w marvin-w commented Aug 29, 2019

@phbaer can you test it with your device?

I don't own a device that has this DPT.

@phbaer

This comment has been minimized.

Copy link
Contributor

@phbaer phbaer commented Aug 29, 2019

@marvin-w will do one I am back home and find some free time. @dstrigl: thanks!

@phbaer

This comment has been minimized.

Copy link
Contributor

@phbaer phbaer commented Sep 2, 2019

@marvin-w , @dstrigl: finally managed to test the PR. It doesn't work for me ... unfortunately. Colour control and status completely useless. I'll try to contact MDT and ask them how their devices are supposed to work.

Will keep you posted.

@farmio farmio added the needs testing label Sep 3, 2019
@dstrigl

This comment has been minimized.

Copy link
Contributor Author

@dstrigl dstrigl commented Sep 3, 2019

@phbaer, that's quite strange ... it look like the two KNX devices behave differently.
My MDT AKD-0424V.02 LED controller works only with the attached pull request.
Let's wait what MDT will say ...

@farmio, let me know if there's somthing I can do ...

@marvin-w

This comment has been minimized.

Copy link
Collaborator

@marvin-w marvin-w commented Sep 3, 2019

We cannot accept this PR if it breaks stuff for other people that before the PR worked fine.

If MDT doesn't supply a useful answer maybe we should just implement a feature toggle of some sort (or implement a reverse DPT) that you can switch between even if that sucks.

@dstrigl

This comment has been minimized.

Copy link
Contributor Author

@dstrigl dstrigl commented Sep 3, 2019

We cannot accept this PR if it breaks stuff for other people that before the PR worked fine.

That's clear!

If MDT doesn't supply a useful answer maybe we should just implement a feature toggle of some
sort (or implement a reverse DPT) that you can switch between even if that sucks.

That would really suck. There should be a clear statement from the KNX Association how the DPT 251.600 should look like.

@marvin-w

This comment has been minimized.

Copy link
Collaborator

@marvin-w marvin-w commented Sep 3, 2019

We cannot accept this PR if it breaks stuff for other people that before the PR worked fine.

That's clear!

If MDT doesn't supply a useful answer maybe we should just implement a feature toggle of some
sort (or implement a reverse DPT) that you can switch between even if that sucks.

That would really suck. There should be a clear statement from the KNX Association how the DPT 251.600 should look like.

True. Honestly, from what I see I believe the PR is actually doing it right. It would also reflect the bitorder in ETS, so lets wait for MDT.

The official KNX specification doesn't contain this DPT, unfortunetly.

@dstrigl

This comment has been minimized.

Copy link
Contributor Author

@dstrigl dstrigl commented Sep 3, 2019

I just got the info from one guy from MDT that this topic (byte order of DPT 251.600) is currently under clarification with the KNX Association.

@dstrigl

This comment has been minimized.

Copy link
Contributor Author

@dstrigl dstrigl commented Sep 3, 2019

Still another statement from the MDT guy (in German):

Die Maskierung in der ETS ist richtig, damit auch der LED Controller. Lediglich die Umrechnung 100% - 0xff ist in der ETS noch falsch.
Grund der Verwirrung, es gibt bei KNX nur ein Vorschlag zum Voting wo die Definition falsch ist und noch kein finales richtiges Dokument.
Die Dali Firmware wird hier nochmal korrigiert werden müssen.

So it sounds like my PR is correct and also the MDT AKD-0424V.02 LED controller is doing it correct 😊.

Just the Dali GW needs a firmware update!

@phbaer

This comment has been minimized.

Copy link
Contributor

@phbaer phbaer commented Sep 3, 2019

Great, thanks for classifying! I'm fine with that change then! :)

@marvin-w

This comment has been minimized.

Copy link
Collaborator

@marvin-w marvin-w commented Sep 3, 2019

Thanks for clarifying. I will approve the PR.

@marvin-w marvin-w closed this Sep 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.