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

Could not parse, APDU length +1 #112

Closed
ascillato opened this issue Feb 20, 2018 · 10 comments
Closed

Could not parse, APDU length +1 #112

ascillato opened this issue Feb 20, 2018 · 10 comments

Comments

@ascillato
Copy link

ascillato commented Feb 20, 2018

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_KNX_IP 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
Copy link
Author

Hi @Julius2342 any help on this?

@Julius2342
Copy link
Collaborator

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

@Julius2342
Copy link
Collaborator

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

@Julius2342
Copy link
Collaborator

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
Copy link
Author

ascillato commented Mar 4, 2018

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
Copy link
Author

ascillato commented Mar 8, 2018

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
Copy link
Author

I could not find any information about this.

The checksum is part of KNX-IP Protocol ?

@Julius2342
Copy link
Collaborator

No idea :)

@DrMurx : do you know this by chance?

@DrMurx
Copy link
Contributor

DrMurx commented Mar 9, 2018

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

@ascillato
Copy link
Author

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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants