From ca8f81f2b4563e355f9034bdd750d2ef497bfb5d Mon Sep 17 00:00:00 2001 From: Grant McEwan Date: Fri, 18 Jun 2021 11:25:10 +1200 Subject: [PATCH 1/6] Allow dcp to respond to identify all filter types IEC 61158-6-10:2019 4.3.1.2 specifies that an identify filter packet can contain 1 or more filter blocks and that any block type can be first. p-net determines that an identify packet is a filter, if the first block is a station name or alias filter block. This fixes pf_dcp_identify_req () so that p-net responds to single or multiple identify filter packets that start any supported filter type. --- src/common/pf_dcp.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/common/pf_dcp.c b/src/common/pf_dcp.c index d859247d..1cd24465 100644 --- a/src/common/pf_dcp.c +++ b/src/common/pf_dcp.c @@ -1440,7 +1440,7 @@ static int pf_dcp_identify_req ( uint16_t ix; bool first = true; /* First of the blocks */ bool match = false; /* Is it for us? */ - bool filter = false; /* Is it IdentifyFilter or IdentifyAll? */ + bool filter = true; /* Is it IdentifyFilter or IdentifyAll? */ uint8_t * p_src; uint16_t src_pos = 0; pf_ethhdr_t * p_src_ethhdr; @@ -1673,18 +1673,18 @@ static int pf_dcp_identify_req ( stationname_position = src_pos; stationname_len = src_block_len; #endif - if ( - (memcmp (p_value, &p_src[src_pos], src_block_len) == 0) && - (p_value[src_block_len] == '\0')) + if (filter == true) { - if (first == true) + if ( + (memcmp (p_value, &p_src[src_pos], src_block_len) != 0) || + (p_value[src_block_len] != '\0')) { - filter = true; + match = false; } } else { - match = false; + ret = -1; } break; case PF_DCP_SUB_DEV_PROP_ID: @@ -1837,7 +1837,6 @@ static int pf_dcp_identify_req ( if (first == true) { p_req_alias_name = alias; - filter = true; } } else From 2b595ad4c399a3f377380e8dc0d7a3a31b24e965 Mon Sep 17 00:00:00 2001 From: Grant McEwan Date: Wed, 23 Jun 2021 15:24:47 +1200 Subject: [PATCH 2/6] Adding gtest for identify by id Adding a gtest for the identify by device id. This does both a check if the device id is correct, and not correct. --- test/test_dcp.cpp | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/test/test_dcp.cpp b/test/test_dcp.cpp index a1e0cb5d..90138546 100644 --- a/test/test_dcp.cpp +++ b/test/test_dcp.cpp @@ -59,6 +59,13 @@ static uint8_t ident_req[] = { 0x73, 0x2d, 0x64, 0x65, 0x6d, 0x6f, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}; +static uint8_t ident_by_device_id_req[] = { + 0x01, 0x0e, 0xcf, 0x00, 0x00, 0x00, 0x74, 0xda, 0x38, 0xc2, 0x5e, 0xa4, + 0x88, 0x92, 0xfe, 0xfe, 0x05, 0x00, 0x01, 0x00, 0x00, 0x01, 0x00, 0x00, + 0x00, 0x08, 0x02, 0x03, 0x00, 0x04, 0xfe, 0xed, 0xbe, 0xef, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}; + static uint8_t set_name_req[] = { 0x12, 0x34, 0x00, 0x78, 0x90, 0xab, 0xc8, 0x5b, 0x76, 0xe6, 0x89, 0xdf, 0x88, 0x92, 0xfe, 0xfd, 0x04, 0x00, 0x00, 0x00, 0x00, 0x03, 0x00, 0x00, @@ -140,6 +147,19 @@ TEST_F (DcpTest, DcpRunTest) ret = pf_eth_recv (mock_os_data.eth_if_handle, net, p_buf); EXPECT_EQ (ret, 1); + TEST_TRACE ("\nGenerating mock set ident by device id request\n"); + p_buf = pnal_buf_alloc (PF_FRAME_BUFFER_SIZE); + memcpy (p_buf->payload, ident_by_device_id_req, sizeof (ident_by_device_id_req)); + ret = pf_eth_recv (mock_os_data.eth_if_handle, net, p_buf); + EXPECT_EQ (ret, 1); + + TEST_TRACE ("\nCheck invalid length set ident by device id request fails\n"); + p_buf = pnal_buf_alloc (PF_FRAME_BUFFER_SIZE); + ident_by_device_id_req[32] = 0x01; + memcpy (p_buf->payload, ident_by_device_id_req, sizeof (ident_by_device_id_req)); + ret = pf_eth_recv (mock_os_data.eth_if_handle, net, p_buf); + EXPECT_EQ (ret, 0); + TEST_TRACE ("\nGenerating mock factory reset request\n"); p_buf = pnal_buf_alloc (PF_FRAME_BUFFER_SIZE); memcpy (p_buf->payload, factory_reset_req, sizeof (factory_reset_req)); From e45ef6a0dd5d94568c0e7c8ef652069ac6e49fe0 Mon Sep 17 00:00:00 2001 From: Grant McEwan Date: Fri, 25 Jun 2021 15:24:20 +1200 Subject: [PATCH 3/6] DCP identify return false if it hasn't handled a request The function pf_dcp_identify_req, would return 1 even if the request wasn't handled by it, this would lead to replies to invalid requests sent to the device. Fixing this so that it only returns 1 if the request being made passes the necessary checks. Fixed the invalid ident_req array in the gtests, filled with proper station name, and tests now check if the station name, dcp data length and dcp block length are correct for packets. --- src/common/pf_dcp.c | 14 +++++++++++++- src/device/pf_cmina.c | 2 +- test/test_dcp.cpp | 2 +- 3 files changed, 15 insertions(+), 3 deletions(-) diff --git a/src/common/pf_dcp.c b/src/common/pf_dcp.c index 1cd24465..4540751c 100644 --- a/src/common/pf_dcp.c +++ b/src/common/pf_dcp.c @@ -1526,8 +1526,15 @@ static int pf_dcp_identify_req ( src_pos += sizeof (*p_src_block_hdr); /* Point to the block value */ src_block_len = ntohs (p_src_block_hdr->block_length); + /* Check if we have a valid dcp data length */ + if (!(src_dcplen >= (src_pos + src_block_len))) + { + ret = -1; + } + else { + match = true; /* So far so good */ + } - match = true; /* So far so good */ while ((ret == 0) && (first || (filter && match)) && (src_dcplen >= (src_pos + src_block_len)) && (dst_pos < PF_FRAME_BUFFER_SIZE)) @@ -1948,6 +1955,11 @@ static int pf_dcp_identify_req ( __LINE__); } + if ((ret <= 0) && (match == false)) + { + return ret; /* Not handled, not matched or something went wrong */ + } + if (p_buf != NULL) { pnal_buf_free (p_buf); diff --git a/src/device/pf_cmina.c b/src/device/pf_cmina.c index 62092cd0..d764d3b0 100644 --- a/src/device/pf_cmina.c +++ b/src/device/pf_cmina.c @@ -1129,7 +1129,7 @@ int pf_cmina_dcp_get_req ( *pp_value = (uint8_t *)net->cmina_current_dcp_ase.product_name; break; case PF_DCP_SUB_DEV_PROP_NAME: - *p_value_length = sizeof (net->cmina_current_dcp_ase.station_name); + *p_value_length = strlen(net->cmina_current_dcp_ase.station_name); *pp_value = (uint8_t *)net->cmina_current_dcp_ase.station_name; break; case PF_DCP_SUB_DEV_PROP_ID: diff --git a/test/test_dcp.cpp b/test/test_dcp.cpp index 90138546..52d647f5 100644 --- a/test/test_dcp.cpp +++ b/test/test_dcp.cpp @@ -54,7 +54,7 @@ static uint8_t get_name_req[] = { static uint8_t ident_req[] = { 0x01, 0x0e, 0xcf, 0x00, 0x00, 0x00, 0xc8, 0x5b, 0x76, 0xe6, 0x89, 0xdf, - 0x88, 0x92, 0xfe, 0xfe, 0x05, 0x00, 0x00, 0x00, 0x00, 0x02, 0x00, 0x01, + 0x88, 0x92, 0xfe, 0xfe, 0x05, 0x00, 0x01, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x10, 0x02, 0x02, 0x00, 0x0c, 0x72, 0x74, 0x2d, 0x6c, 0x61, 0x62, 0x73, 0x2d, 0x64, 0x65, 0x6d, 0x6f, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}; From 2c6b4f92d1bab4cba506e853924bd9565603eb15 Mon Sep 17 00:00:00 2001 From: Grant McEwan Date: Wed, 23 Jun 2021 15:24:47 +1200 Subject: [PATCH 4/6] Adding gtest for identify by id Adding a gtest for the identify by device id. This does both a check if the device id is correct, and not correct. --- test/test_dcp.cpp | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/test/test_dcp.cpp b/test/test_dcp.cpp index 52d647f5..0dddf351 100644 --- a/test/test_dcp.cpp +++ b/test/test_dcp.cpp @@ -149,14 +149,20 @@ TEST_F (DcpTest, DcpRunTest) TEST_TRACE ("\nGenerating mock set ident by device id request\n"); p_buf = pnal_buf_alloc (PF_FRAME_BUFFER_SIZE); - memcpy (p_buf->payload, ident_by_device_id_req, sizeof (ident_by_device_id_req)); + memcpy ( + p_buf->payload, + ident_by_device_id_req, + sizeof (ident_by_device_id_req)); ret = pf_eth_recv (mock_os_data.eth_if_handle, net, p_buf); EXPECT_EQ (ret, 1); TEST_TRACE ("\nCheck invalid length set ident by device id request fails\n"); p_buf = pnal_buf_alloc (PF_FRAME_BUFFER_SIZE); ident_by_device_id_req[32] = 0x01; - memcpy (p_buf->payload, ident_by_device_id_req, sizeof (ident_by_device_id_req)); + memcpy ( + p_buf->payload, + ident_by_device_id_req, + sizeof (ident_by_device_id_req)); ret = pf_eth_recv (mock_os_data.eth_if_handle, net, p_buf); EXPECT_EQ (ret, 0); From 5644cb58fa3b0759055278bb24053abf983eb6c1 Mon Sep 17 00:00:00 2001 From: Grant McEwan Date: Tue, 27 Jul 2021 10:15:23 +1200 Subject: [PATCH 5/6] Count packet types sent in mock pnal_eth_send for dcp tests The DCP tests would count packets at the end to ensure that p-net did actually reply, this count however included LLDP packets since LLDP is initiated at the start of the test, and is not really able to be turned off. Also add a small delay between sending identity requests so that the stack has time to add the outgoing messages to the queue, otherwise the 2nd message stomps the first. --- test/mocks.cpp | 38 +++++++++++++++++++++++++++++++++++++- test/mocks.h | 2 ++ test/test_dcp.cpp | 20 +++++++++++++++++--- 3 files changed, 56 insertions(+), 4 deletions(-) diff --git a/test/mocks.cpp b/test/mocks.cpp index 9034ba29..cd0b6312 100644 --- a/test/mocks.cpp +++ b/test/mocks.cpp @@ -130,12 +130,48 @@ int mock_pnal_set_ip_suite ( return 0; } +static uint16_t mock_get_eth_packet_type (pnal_buf_t * p_buf) +{ + uint16_t eth_type_pos = 2 * sizeof (pnet_ethaddr_t); + uint16_t eth_type = 0; + const uint16_t * p_data = NULL; + + /* Skip ALL VLAN tags */ + p_data = (uint16_t *)(&((uint8_t *)p_buf->payload)[eth_type_pos]); + eth_type = ntohs (p_data[0]); + while (eth_type == PNAL_ETHTYPE_VLAN) + { + eth_type_pos += 4; /* Sizeof VLAN tag */ + + p_data = (uint16_t *)(&((uint8_t *)p_buf->payload)[eth_type_pos]); + eth_type = ntohs (p_data[0]); + } + + return eth_type; +} + int mock_pnal_eth_send (pnal_eth_handle_t * handle, pnal_buf_t * p_buf) { + uint16_t eth_type = 0; + + eth_type = mock_get_eth_packet_type (p_buf); + memcpy (mock_os_data.eth_send_copy, p_buf->payload, p_buf->len); mock_os_data.eth_send_len = p_buf->len; - mock_os_data.eth_send_count++; + /* Count packet types sent, so we can differentiate between them */ + switch (eth_type) + { + case PNAL_ETHTYPE_LLDP: + mock_os_data.eth_lldp_count++; + break; + case PNAL_ETHTYPE_PROFINET: + mock_os_data.eth_profinet_count++; + default: + break; + } + + mock_os_data.eth_send_count++; return p_buf->len; } diff --git a/test/mocks.h b/test/mocks.h index f5c22bf7..fb18c83f 100644 --- a/test/mocks.h +++ b/test/mocks.h @@ -32,6 +32,8 @@ typedef struct mock_os_data_obj uint8_t eth_send_copy[PF_FRAME_BUFFER_SIZE]; uint16_t eth_send_len; uint16_t eth_send_count; + uint16_t eth_lldp_count; + uint16_t eth_profinet_count; /* Per port Ethernet link status. * Note that port numbers start at 1. To simplify test cases, we add a diff --git a/test/test_dcp.cpp b/test/test_dcp.cpp index 0dddf351..2da9cc28 100644 --- a/test/test_dcp.cpp +++ b/test/test_dcp.cpp @@ -128,24 +128,30 @@ TEST_F (DcpTest, DcpRunTest) { pnal_buf_t * p_buf; int ret; + int successful_replies = 0; TEST_TRACE ("\nGenerating mock set name request\n"); p_buf = pnal_buf_alloc (PF_FRAME_BUFFER_SIZE); memcpy (p_buf->payload, set_name_req, sizeof (set_name_req)); ret = pf_eth_recv (mock_os_data.eth_if_handle, net, p_buf); EXPECT_EQ (ret, 1); + successful_replies += ret; TEST_TRACE ("\nGenerating mock set IP request\n"); p_buf = pnal_buf_alloc (PF_FRAME_BUFFER_SIZE); memcpy (p_buf->payload, set_ip_req, sizeof (set_ip_req)); ret = pf_eth_recv (mock_os_data.eth_if_handle, net, p_buf); EXPECT_EQ (ret, 1); + successful_replies += ret; TEST_TRACE ("\nGenerating mock set ident request\n"); p_buf = pnal_buf_alloc (PF_FRAME_BUFFER_SIZE); memcpy (p_buf->payload, ident_req, sizeof (ident_req)); ret = pf_eth_recv (mock_os_data.eth_if_handle, net, p_buf); EXPECT_EQ (ret, 1); + successful_replies += ret; + + run_stack (2 * 1000); TEST_TRACE ("\nGenerating mock set ident by device id request\n"); p_buf = pnal_buf_alloc (PF_FRAME_BUFFER_SIZE); @@ -155,6 +161,7 @@ TEST_F (DcpTest, DcpRunTest) sizeof (ident_by_device_id_req)); ret = pf_eth_recv (mock_os_data.eth_if_handle, net, p_buf); EXPECT_EQ (ret, 1); + successful_replies += ret; TEST_TRACE ("\nCheck invalid length set ident by device id request fails\n"); p_buf = pnal_buf_alloc (PF_FRAME_BUFFER_SIZE); @@ -171,18 +178,25 @@ TEST_F (DcpTest, DcpRunTest) memcpy (p_buf->payload, factory_reset_req, sizeof (factory_reset_req)); ret = pf_eth_recv (mock_os_data.eth_if_handle, net, p_buf); EXPECT_EQ (ret, 1); + successful_replies += ret; TEST_TRACE ("\nGenerating mock flash LED request\n"); p_buf = pnal_buf_alloc (PF_FRAME_BUFFER_SIZE); memcpy (p_buf->payload, signal_req, sizeof (signal_req)); ret = pf_eth_recv (mock_os_data.eth_if_handle, net, p_buf); EXPECT_EQ (ret, 1); + successful_replies += ret; + /* Wait for LED to flash three times at 1 Hz */ run_stack (4 * 1000 * 1000); - EXPECT_EQ ( - mock_os_data.eth_send_count, - 9 + (PNET_MAX_PHYSICAL_PORTS - 1) * 4); + TEST_TRACE ( + "Pkts Recv, %d lldp, %d profinet, %d total\n", + mock_os_data.eth_lldp_count, + mock_os_data.eth_profinet_count, + mock_os_data.eth_send_count); + + EXPECT_EQ (mock_os_data.eth_profinet_count, successful_replies); EXPECT_EQ (mock_os_data.set_ip_suite_count, 2); EXPECT_EQ (appdata.call_counters.led_on_calls, 3); From 20edb1f5a887e5fdd75803686fdb6e39265eacef Mon Sep 17 00:00:00 2001 From: Grant McEwan Date: Mon, 11 Oct 2021 14:26:34 +1300 Subject: [PATCH 6/6] Fix failed test when using --gtest_repeat Using the feature --gtest_repeat would fail for the dcp test as the new test introduced would change a byte of the array to make sure that the device ID was not recognized. But when running the test again on repeat, the array would stay modified, and the test fail because the ID was incorrect. --- test/test_dcp.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/test_dcp.cpp b/test/test_dcp.cpp index 2da9cc28..0593e8c7 100644 --- a/test/test_dcp.cpp +++ b/test/test_dcp.cpp @@ -129,6 +129,7 @@ TEST_F (DcpTest, DcpRunTest) pnal_buf_t * p_buf; int ret; int successful_replies = 0; + int changed_byte = 0; TEST_TRACE ("\nGenerating mock set name request\n"); p_buf = pnal_buf_alloc (PF_FRAME_BUFFER_SIZE); @@ -165,12 +166,14 @@ TEST_F (DcpTest, DcpRunTest) TEST_TRACE ("\nCheck invalid length set ident by device id request fails\n"); p_buf = pnal_buf_alloc (PF_FRAME_BUFFER_SIZE); + changed_byte = ident_by_device_id_req[32]; ident_by_device_id_req[32] = 0x01; memcpy ( p_buf->payload, ident_by_device_id_req, sizeof (ident_by_device_id_req)); ret = pf_eth_recv (mock_os_data.eth_if_handle, net, p_buf); + ident_by_device_id_req[32] = changed_byte; EXPECT_EQ (ret, 0); TEST_TRACE ("\nGenerating mock factory reset request\n");