Add echo example apps to conduct end-to-end connectivity sanity check… #3836

yufengwangca · 2020-11-13T15:57:56Z

… on protocol level

Problem

Right now, we don't have an end-to-end testing for ExchangeManager/CRMP in CHIP, we are not able to detect the regression or break on the protocol level. After Echo protocol landed, we need test mock apps to:

Demonstrate how to write an app against a CHIP protocol
To conduct sanity connectivity check on protocol level for Happy/Cirque

Summary of Changes

Add echo example apps to conduct end-to-end connectivity sanity check.

Fixes #3665

examples/echo/common.cpp

examples/echo/echo_requester.cpp

… on protocol level

examples/echo/README.md

bzbarsky-apple · 2020-11-14T02:32:54Z

examples/echo/common.h

+constexpr chip::NodeId kClientDeviceId = 12344321;
+constexpr chip::NodeId kServerDeviceId = 12344322;


Can these use kTestControllerNodeId and kTestDeviceNodeId from src/transport/SecurePairingSession.h?

This is served as an example app, so I prefer to allow the developer to modify the test Node ID, and the example app should be programmed against the high level public stack header files much as possible.

I'm not sure I follow. The whole point of those test ids is to be used from example apps, and other example apps use them right now. If they are not exposed in a "public" header, they should move there.

examples/echo/echo_requester.cpp

bzbarsky-apple · 2020-11-14T02:37:55Z

examples/echo/echo_responder.cpp

+// Callback handler when a CHIP EchoRequest is received.
+static void HandleEchoRequestReceived(NodeId nodeId, System::PacketBuffer * payload)
+{
+    printf("Echo Request from node %lu, len=%u ... sending response.\n", nodeId, payload->DataLength());


The README makes it sound like responses get sent....

Yes, this callback allows the app to take more actions upon receiving the echo request, the echo response is sent by Echo protocol automatically. In this example, app just print out a message.

examples/echo/echo_requester.cpp

examples/echo/third_party/connectedhomeip

rwalker-apple

I really thought that we'd have a standalone unit test for ExchangeMgr using echo (or another arbitrary protocol ID).

examples/ subdirectory entries should all look like customers of CHIP (separable from the tree once CHIP has a stable API). As such, they should not be "the only test we have" for CHIP library code.

Can you please investigate making this manifest under src/messaging/tests/ ?

yufengwangca · 2020-11-17T02:33:33Z

I really thought that we'd have a standalone unit test for ExchangeMgr using echo (or another arbitrary protocol ID).

examples/ subdirectory entries should all look like customers of CHIP (separable from the tree once CHIP has a stable API). As such, they should not be "the only test we have" for CHIP library code.

Can you please investigate making this manifest under src/messaging/tests/ ?

'src/messaging/tests/' currently is used for unit testing of the messaging APIs, this standalone app is created against whole CHIP stack, so I feel it should not be put inside CHIP stack.

Since the users of this app is happy/cirque automation frameworks, which are also outside of src directory, does it make more sense to put this app under "integrations" directory?

andy31415 · 2020-11-18T22:06:23Z

I believe unit tests have great value for new code. Failure in integration tests are much harder to track down than focused unit tests.

mspang

I really thought that we'd have a standalone unit test for ExchangeMgr using echo (or another arbitrary protocol ID).
examples/ subdirectory entries should all look like customers of CHIP (separable from the tree once CHIP has a stable API). As such, they should not be "the only test we have" for CHIP library code.
Can you please investigate making this manifest under src/messaging/tests/ ?

'src/messaging/tests/' currently is used for unit testing of the messaging APIs, this standalone app is created against whole CHIP stack, so I feel it should not be put inside CHIP stack.

There's no problem adding even integration tests to the main tree. It just depends on whether the main build with chip_build_tests=true should build it, or not.

Doesn't seem true that this uses the whole CHIP stack; it doesn't depend on any components that messaging doesn't use (but it's not really a problem for a test to use a more components than the code under test, such as test_support libraries).

Since the users of this app is happy/cirque automation frameworks, which are also outside of src directory, does it make more sense to put this app under "integrations" directory?

src/messaging/tests/echo/BUILD.gn

src/messaging/tests/echo/.gn

…I changes

yufengwangca · 2020-12-02T20:31:36Z

Please help to review this PR, messaging layer have been broken twice in last three weeks, we do need to add integration test with this app into CI to prevent those regression in future.

#3984
#3920

rwalker-apple · 2020-12-07T20:06:13Z

src/messaging/tests/echo/common.h

+
+#pragma once
+
+#include <core/CHIPCore.h>


this header doesn't appear to need all of these include directives, they should be moved to the source files that need 'em

rwalker-apple · 2020-12-07T20:07:01Z

src/messaging/tests/echo/echo_requester.cpp

+// Count of the number of EchoResponses received.
+uint64_t gEchoRespCount = 0;
+
+uint64_t Now(void)


why not use the CHIP notion of "now"?

rwalker-apple · 2020-12-07T20:07:52Z

src/messaging/tests/echo/echo_requester.cpp

+        int32_t len = snprintf(p, CHIP_SYSTEM_CONFIG_HEADER_RESERVE_SIZE, "Echo Message %" PRIu64 "\n", gEchoCount);
+
+        // Set the datalength in the buffer appropriately.
+        payloadBuf->SetDataLength((uint16_t) len);


we have checks for the cast-ability of these values, please use them?

And C++-style cast.

rwalker-apple · 2020-12-07T20:09:17Z

src/messaging/tests/echo/echo_requester.cpp

+
+    err = gTransportManager.Init(Transport::UdpListenParameters(&DeviceLayer::InetLayer)
+                                     .SetAddressType(Inet::kIPAddressType_IPv4)
+                                     .SetListenPort(ECHO_CLIENT_PORT));


is this SetListenPort() required? can the client be on an ephemeral port?

This is to make sure the client get set to a different port other than default CHIP_PORT, we need this if we want to test echo on the same machine.

mspang · 2020-12-08T01:40:28Z

src/messaging/tests/echo/common.h

+
+#include <messaging/ExchangeMgr.h>
+
+using namespace chip;


Please don't use using in headers.

mspang · 2020-12-08T01:55:33Z

src/messaging/tests/echo/README.md

+
+```
+source scripts/activate.sh
+cd src/messaging/tests/echo


Suggest removing this line, there is no difference building in src/messaging/tests/echo vs building at the top level so this may be confusing or waste disk space.

jelderton · 2020-12-08T16:55:47Z

src/messaging/tests/echo/common.cpp

+{
+    CHIP_ERROR err = CHIP_NO_ERROR;
+
+    printf("Init CHIP Stack\r\n");


nit: the carriage return + line feed is in this function but not other places.

boring-cyborg bot added the examples label Nov 13, 2020

pullapprove bot requested review from andy31415, BroderickCarlin, bzbarsky-apple, chrisdecenzo, hawk248, jelderton, mspang, rwalker-apple and saurabhst November 13, 2020 15:58

yufengwangca requested a review from erjiaqing November 13, 2020 16:02

andy31415 approved these changes Nov 13, 2020

View reviewed changes

yufengwangca added 2 commits November 13, 2020 11:00

Add echo example apps to conduct end-to-end connectivity sanity check…

6253d56

… on protocol level

Address review comments

030db40

bzbarsky-apple reviewed Nov 14, 2020

View reviewed changes

tcarmelveilleux reviewed Nov 14, 2020

View reviewed changes

examples/echo/echo_requester.cpp Outdated Show resolved Hide resolved

tcarmelveilleux reviewed Nov 14, 2020

View reviewed changes

examples/echo/third_party/connectedhomeip Outdated Show resolved Hide resolved

Using c++ style of conversion

bfa3aba

rwalker-apple suggested changes Nov 16, 2020

View reviewed changes

erjiaqing mentioned this pull request Nov 17, 2020

[rendezvous] Add initial support for IP rendezvous #3704

Merged

Relocate echo from example to src/messaging/tests

48fa62e

yufengwangca requested a review from rwalker-apple November 17, 2020 23:48

andy31415 self-requested a review November 18, 2020 22:05

mspang reviewed Nov 19, 2020

View reviewed changes

src/messaging/tests/echo/BUILD.gn Outdated Show resolved Hide resolved

mspang requested changes Nov 19, 2020

View reviewed changes

src/messaging/tests/echo/.gn Outdated Show resolved Hide resolved

yufengwangca added 3 commits November 18, 2020 17:07

Update due to system API update

8ed1db5

Remvoe BUILD_RELEASE from build.gn

438f690

Update SecureSessionManager init due to API changes in Transport

61e68b7

restyled-io bot mentioned this pull request Dec 2, 2020

Restyle Add echo example apps to conduct end-to-end connectivity sanity check… #4058

Closed

Remove extra build constructs and update against the latest system AP…

27fbbde

…I changes

yufengwangca requested a review from mspang December 2, 2020 19:48

yufengwangca requested a review from yunhanw-google December 4, 2020 21:59

yunhanw-google approved these changes Dec 7, 2020

View reviewed changes

rwalker-apple reviewed Dec 7, 2020

View reviewed changes

rwalker-apple approved these changes Dec 7, 2020

View reviewed changes

Address review comments

a0b5d07

mspang approved these changes Dec 8, 2020

View reviewed changes

Remove 'using chip' from common.h

360fd58

jelderton reviewed Dec 8, 2020

View reviewed changes

jelderton approved these changes Dec 8, 2020

View reviewed changes

jelderton merged commit 4282a07 into project-chip:master Dec 8, 2020

bzbarsky-apple mentioned this pull request Dec 9, 2020

Fix Mac build breakage from incorrect formats for node ids #4139

Merged

yufengwangca deleted the pr/examples/echo branch December 10, 2020 18:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add echo example apps to conduct end-to-end connectivity sanity check… #3836

Add echo example apps to conduct end-to-end connectivity sanity check… #3836

yufengwangca commented Nov 13, 2020 •

edited

Loading

bzbarsky-apple Nov 14, 2020

yufengwangca Nov 16, 2020 •

edited

Loading

bzbarsky-apple Dec 7, 2020

yufengwangca Dec 8, 2020

bzbarsky-apple Nov 14, 2020

yufengwangca Nov 16, 2020 •

edited

Loading

rwalker-apple left a comment

yufengwangca commented Nov 17, 2020

andy31415 commented Nov 18, 2020

mspang left a comment

yufengwangca commented Dec 2, 2020

rwalker-apple Dec 7, 2020

yufengwangca Dec 8, 2020

rwalker-apple Dec 7, 2020

yufengwangca Dec 8, 2020

rwalker-apple Dec 7, 2020

bzbarsky-apple Dec 7, 2020

yufengwangca Dec 8, 2020

rwalker-apple Dec 7, 2020 •

edited

Loading

yufengwangca Dec 8, 2020

mspang Dec 8, 2020

yufengwangca Dec 8, 2020

mspang Dec 8, 2020

yufengwangca Dec 8, 2020

jelderton Dec 8, 2020

		constexpr chip::NodeId kClientDeviceId = 12344321;
		constexpr chip::NodeId kServerDeviceId = 12344322;


		#pragma once

		#include <core/CHIPCore.h>

Add echo example apps to conduct end-to-end connectivity sanity check… #3836

Add echo example apps to conduct end-to-end connectivity sanity check… #3836

Conversation

yufengwangca commented Nov 13, 2020 • edited Loading

Problem

Summary of Changes

Choose a reason for hiding this comment

yufengwangca Nov 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yufengwangca Nov 16, 2020 • edited Loading

Choose a reason for hiding this comment

rwalker-apple left a comment

Choose a reason for hiding this comment

yufengwangca commented Nov 17, 2020

andy31415 commented Nov 18, 2020

mspang left a comment

Choose a reason for hiding this comment

yufengwangca commented Dec 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwalker-apple Dec 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yufengwangca commented Nov 13, 2020 •

edited

Loading

yufengwangca Nov 16, 2020 •

edited

Loading

yufengwangca Nov 16, 2020 •

edited

Loading

rwalker-apple Dec 7, 2020 •

edited

Loading