XKNX / xknx

XKNX - A KNX library written in Python
http://xknx.io/
MIT License
289 stars 105 forks source link

Could not parse, APDU length +1 #112

Closed ascillato closed 6 years ago

ascillato commented 6 years ago

Hi,

Great software!

I'm using XKNX v0.8.2 on Home Assistant v0.64.3. XKNX connects to a KNXd server on the same Raspberry Pi than Home Assistant. I'm using _ESP_KNXIP on a NodeMCU.

My problem is that using the ESP_KNX_IP as a KNX Switch, XKNX say:

<CouldNotParseKNXIP description="APDU LEN should be 1 but is 2" />
Traceback (most recent call last):
  File "/srv/homeassistant/lib/python3.5/site-packages/xknx/io/udp_client.py", line 85, in data_received_callback
    knxipframe.from_knx(raw)
  File "/srv/homeassistant/lib/python3.5/site-packages/xknx/knxip/knxip.py", line 78, in from_knx
    pos += self.body.from_knx(data[pos:])
  File "/srv/homeassistant/lib/python3.5/site-packages/xknx/knxip/cemi_frame.py", line 116, in from_knx
    return self.from_knx_data_link_layer(raw)
  File "/srv/homeassistant/lib/python3.5/site-packages/xknx/knxip/cemi_frame.py", line 153, in from_knx_data_link_layer
    self.mpdu_len, len(apdu)))
xknx.exceptions.exception.CouldNotParseKNXIP: <CouldNotParseKNXIP description="APDU LEN should be 1 but is 2" />

But the KNX protocol is like this:

    +---------+--------+--------+--------+--------+---------+---------+--------+---------+
    | Header  |  Msg   |Add.Info| Ctrl 1 | Ctrl 2 | Source  | Dest.   |  Data  |   APDU  |
    |         | Code   | Length |        |        | Address | Address | Length |         |
    +---------+--------+--------+--------+--------+---------+---------+--------+---------+
      6 bytes   1 byte   1 byte   1 byte   1 byte   2 bytes   2 bytes   1 byte   2 bytes

        Header          = See below the structure of a cEMI header
        Message Code    = See below. On Appendix A is the list of all existing EMI and cEMI codes
        Add.Info Length = 0x00 - no additional info
        Control Field 1 =
        Control Field 2 =
        Source Address  = 0x0000 - filled in by router/gateway with its source address which is
                          part of the KNX subnet
        Dest. Address   = KNX group or individual address (2 byte)
        Data Length     = Number of bytes of data in the APDU excluding the TPCI/APCI bits
        APDU            = Application Protocol Data Unit - the actual payload including transport
                          protocol control information (TPCI), application protocol control
                          information (APCI) and data passed as an argument from higher layers of
                          the KNX communication stack

So, if APDU should be 2 bytes, why XKNX expects 1 byte?

If I modify the ESP_KNX_IP code to send 1 APDU Byte, XKNX recognices the telegram sent by ESP_KNX_IP without error. The ESP_KNX_IP recognices the messages from XKNX without problem.

What I'm missing?

Thanks

ascillato commented 6 years ago

Hi @Julius2342 any help on this?

Julius2342 commented 6 years ago

Hi @ascillato : to be honest, i did not really have a look at it yet ... I was pretty busy with work and family.

Julius2342 commented 6 years ago

may you print out the packet which fails? Bc then we can write a unit test and fix it against it?

Julius2342 commented 6 years ago

i just looked at the code ( xknx/knxip/cemi_frame.py ). The Len is taken from here:

self.mpdu_len = cemi[8 + addil]

addil (additional information length) is usually 0, at least i have never seen anything != 0.

So the mdpu_len should point directly to what is described as "Data Length" in your description.

ascillato commented 6 years ago

Hi. Thanks for the reply. There is no rush with this. I got my devices working without problem when I reprogram my device to send the actual UDP package but with len -1.

When I have that error on XKNX, the console output of my device say:

T: -99.00°C  H: -99.00%
Creating packet with len 20
Sending packet: 0x6 0x10 0x5 0x30 0x0 0x14 0x29 0x0 0xBC 0xE0 0x11 0x1 0x21 0x1 0x3 0x0 0x80 0x9B 0x2A 0x40
Creating packet with len 20
Sending packet: 0x6 0x10 0x5 0x30 0x0 0x14 0x29 0x0 0xBC 0xE0 0x11 0x1 0x21 0x2 0x3 0x0 0x80 0x9B 0x2A 0x43

I found that the ESP_KNX_IP Library send the package with a Checksum at the end. When I made that the UDP package that my device sent, be the same but without the last character, I was taking out the CheckSum Byte.

That's why XKNX was not able to process the last byte with the unmodified ESP_KNX_Library. The last Byte was not an APDU part, was a Checksum.

So, What do you think? It's a job for XKNX to read the checksum or ESP_KNX_IP Library should eliminate it?

ascillato commented 6 years ago

Following is the code for sending the KNX Telegram on ESP_KNX_IP Library. Here you can see that it sends the telegram + checksum.

@sisamiwe have checked that the ESP_KNX_IP library works on a KNX Installation.

So, @Julius2342 , XKNX should check the checksum?

void ESPKNXIP::send(address_t const &receiver, knx_command_type_t ct, uint8_t data_len, uint8_t *data)
{
    if (receiver.value == 0)
    return;

    uint32_t len = 6 + 2 + 8 + data_len + 1; // knx_pkt + cemi_msg + cemi_service + data + checksum
    DEBUG_PRINT(F("Creating packet with len "));
    DEBUG_PRINTLN(len)
    uint8_t buf[len];
    knx_ip_pkt_t *knx_pkt = (knx_ip_pkt_t *)buf;
    knx_pkt->header_len = 0x06;
    knx_pkt->protocol_version = 0x10;
    knx_pkt->service_type = __ntohs(KNX_ST_ROUTING_INDICATION);
    knx_pkt->total_len.len = __ntohs(len);
    cemi_msg_t *cemi_msg = (cemi_msg_t *)knx_pkt->pkt_data;
    cemi_msg->message_code = KNX_MT_L_DATA_IND;
    cemi_msg->additional_info_len = 0;
    cemi_service_t *cemi_data = &cemi_msg->data.service_information;
    cemi_data->control_1.bits.confirm = 0;
    cemi_data->control_1.bits.ack = 0;
    cemi_data->control_1.bits.priority = B11;
    cemi_data->control_1.bits.system_broadcast = 0x01;
    cemi_data->control_1.bits.repeat = 0x01;
    cemi_data->control_1.bits.reserved = 0;
    cemi_data->control_1.bits.frame_type = 0x01;
    cemi_data->control_2.bits.extended_frame_format = 0x00;
    cemi_data->control_2.bits.hop_count = 0x06;
    cemi_data->control_2.bits.dest_addr_type = 0x01;
    cemi_data->source = physaddr;
    cemi_data->destination = receiver;
    //cemi_data->destination.bytes.high = (area << 3) | line;
    //cemi_data->destination.bytes.low = member;
    cemi_data->data_len = data_len;
    cemi_data->pci.apci = (ct & 0x0C) >> 2;
    cemi_data->pci.tpci_seq_number = 0x00; // ???
    cemi_data->pci.tpci_comm_type = KNX_COT_UDP; // ???
    memcpy(cemi_data->data, data, data_len);
    cemi_data->data[0] = (cemi_data->data[0] & 0x3F) | ((ct & 0x03) << 6);

    // Calculate checksum, which is just XOR of all bytes
    uint8_t cs = buf[0] ^ buf[1];
    for (uint32_t i = 2; i < len - 1; ++i)
    {
        cs ^= buf[i];
    }
    buf[len - 1] = cs;

    DEBUG_PRINT(F("Sending packet:"));
    for (int i = 0; i < len; ++i)
    {
        DEBUG_PRINT(F(" 0x"));
        DEBUG_PRINT(buf[i], 16);
    }
    DEBUG_PRINTLN(F(""));

    udp.beginPacketMulticast(MULTICAST_IP, MULTICAST_PORT, WiFi.localIP());
    udp.write(buf, len);
    udp.endPacket();
}

If I change the line udp.write(buf, len); for udp.write(buf, len-1); the XKNX Library works very good and very fast. But I'm taking out the Checksum byte with this.

The checksum is part of KNX-IP Protocol?

ascillato commented 6 years ago

I could not find any information about this.

The checksum is part of KNX-IP Protocol ?

Julius2342 commented 6 years ago

No idea :)

@DrMurx : do you know this by chance?

DrMurx commented 6 years ago

Not from the back of my head. I'll try to wrap my head around this on the weekend.

ascillato commented 6 years ago

Could not find any info about this, but the author of the KNX Library for ESP8266 (envy) agreed to eliminate the checksum, so now the library works great with XKNX and Home Assistant.

Thanks

Closing issue...