Fix hal-sleep/sleep_manager Tests on Cypress Targets #13210

dustin-crossman · 2020-06-30T22:49:57Z

Summary of changes

This fixes the mbed_hal-sleep and mbed_hal-sleep_manager tests that have been failing on Cypress targets.
See this issue for background: #12434
In particular my comment here has an explanation of the failures: #12434 (comment)
This PR would fix the first 2 issues from that comment. The third issue was fixed by #12978

Impact of changes

Migration actions required

Documentation

None

Pull request type

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Test Reports:
GT_FT_KIT_062_BLE_GCC.txt
GT_FT_KIT_062_WIFI_BT_GCC.txt
GT_FT_KIT_062S2_43012_GCC.txt
GT_FT_P6S1_43012EVB_01_GCC.txt
GT_FT_P6S1_43438EVB_01_GCC.txt
GT_FT_PROTO_062_4343W_GCC.txt

Two types of failures are seen:

cordio_hci-driver test on 4343W boards: This is a recent failure we've seen and is being investigated.
Intermittent timeouts: Looking closer at the logs these are a pyocd issue (our test setup uses the pyocd copy method) not legitimate failures.

Reviewers

ciarmcom · 2020-06-30T23:00:29Z

@dustin-crossman, thank you for your changes.
@ARMmbed/mbed-os-test @ARMmbed/mbed-os-hal @ARMmbed/mbed-os-maintainers please review.

0xc0170 · 2020-07-01T07:52:17Z

@dustin-crossman Can you extend the commit messages to contain details how is this fixing and what is it fixing? additional paragraph would be good to have there with details

0xc0170

Additional details in the commit messages

jamesbeyond · 2020-07-01T09:13:42Z

features/frameworks/greentea-client/source/greentea_test_env.cpp

@@ -523,7 +523,7 @@ static int CurTok = 0;
 *
 *        tok_eof       ::= EOF (end of file)
 *        tok_open      ::= "{{"
- *        tok_close     ::= "}}"
+ *        tok_close     ::= "}}\n"


Why this fix to Cypress targets need to change the behavior of greentea?
I doubt there will be unexpected consequences.
The Greentea message {{KEY;VALUE}} is bidirectional, HOST<-->TARGET
What if the HOST is windows sending {{KEY;VALUE}}\r\n to target?

As described here: #12434 (comment) this is a long standing green tea bug, though as far as I know it only caused failures on our targets.
The choice of line ending in the case of greentea shouldn't have anything to do with the host OS. It appears to be merely convention that greentea KV pairs are sent with the '\n'. See: https://github.com/ARMmbed/mbed-os-tools/blob/master/src/mbed_os_tools/test/host_tests_conn_proxy/conn_primitive.py#L39
I don't see any reason why a Greentea message would be sent with a \r\n ending unless someone was perhaps manually testing messages with a serial emulator but had the wrong settings.

Mbed greentea sends all KV pairs in the following format:
"{{key,value}}\n"
Previously the greentea client code was not reading the final '\n' from
the UART as part of the HandleKV() call. This did not cause an
error when multiple KV pairs were being parsed because any whitespace
proceeding a message was ignored. Once the final KV pair was sent,
however, HandleKV() would only read up to the "}}" leaving the final
'\n' in the UART buffer.

The commit msg now should explain it. As the host is sending \n we should read it as well.

Hi @dustin-crossman,
Thanks for your updates. now I can fully understood the reason behind the change.
And I tried your changes, it is working on the automated test, but manual test on windows is not working as we expected. But I think manual test it is essential use case, I personally used it a lot during writing/debugging tests.

I am not sure why Greentea python tool put that \n in the message in first place, maybe trying forcing flash the TX buffer? So lets leave it there as it is.

So I'd like to propose the changes to:

in the doxy/docs we keep the end token to }}

in the code, you can let Greentea to read another char, but will ignore it rather than restrict it to \n

then add a inline comment, saying this to to matching with GT python code where you pointed out.

By doing this.
GT c++ is aligned with the GT python code
Your targets will have cleared RX buffer.
and manual test on windows or any platform will not get impact.
Also behavior of greentea unchanged ,and No docs/API updates are required.

What do you think?

I'm not sure I really understand your goal here. I would think that the python greentea tool is the definitive place to look at for the format of a KV pair which states that a kv pair has a \n ending. Why not document that and keep all tools compliant with that?

It sounds like your manual tests are failing because they are sending \r\n. Why not fix that? Most serial emulators I've used have configurable line endings (on Windows or otherwise).

Hi @dustin-crossman
My main goal here is try to fix the inconsistency of GT python, and GT C++ part (let your target pass), and reduced the impacts all GT users at same time.

It is not just me that using manual testing for GT, many other people also use it on windows. This change would break them as well. I would NOT expect all of them go and configure their windows serial terminals.

change the end token would means change behavior, that would means all the documents related to it need to be updated.
https://github.com/ARMmbed/mbed-os-5-docs/blob/development/docs/debugging-testing/testing/testing_greentea.md
https://github.com/ARMmbed/mbed-os-tools/tree/master/packages/mbed-greentea
https://github.com/ARMmbed/mbed-os-tools/tree/master/packages/mbed-host-tests
And possibly a new release of GT python tool.
That is huge trouble, not mention exist windows users might be stuck on this inconspicuous change looking for why.

I would say what I suggested is minimal changes with least effort, And resolved your targets failure, and no one got impacted.

Mbed greentea sends all KV pairs in the following format: "{{key,value}}\n" Previously the greentea client code was not reading the final '\n' from the UART as part of the HandleKV() call. This did not cause an error when multiple KV pairs were being parsed because any whitespace proceeding a message was ignored. Once the final KV pair was sent, however, HandleKV() would only read up to the "}}" leaving the final '\n' in the UART buffer.

During the sleep_usticker_test and the deepsleep_lpticker_test a sleep was placed above the main testing loop in order to give the device some time to finish the Green Tea UART transmissions before entering sleep mode. Since there is a Green Tea transmission at the end of each iteration of this test loop, this sleep should be called for each loop instead of just once before it.

Pull request has been modified.

dustin-crossman · 2020-07-01T18:01:12Z

@0xc0170 Added more details. Let me know if that looks good!

0xc0170 · 2020-07-02T14:10:02Z

Thank for the updated commit messages, looks fine to me now

0xc0170 · 2020-07-02T14:55:53Z

CI run meanwhile

mbed-ci · 2020-07-02T16:48:51Z

Test run: SUCCESS

Summary: 6 of 6 test jobs passed
Build number : 1
Build artifacts

0xc0170 · 2020-07-03T08:38:45Z

It was not "ready for merge", not cool 😞 I realized right after merge.

@jamesbeyond Please review and I can revert asap the second commit if there are issues and can be fixed via a new PR.

jamesbeyond · 2020-07-03T08:51:46Z

Hi @0xc0170, I believe the discussion is on going, could you revert this one?

0xc0170 · 2020-07-03T08:56:06Z

@dustin-crossman I'll revert the commit on master soon. Please send a new PR and lets review it.

ciarmcom requested review from a team June 30, 2020 23:00

ciarmcom added the needs: review label Jun 30, 2020

0xc0170 previously requested changes Jul 1, 2020

View reviewed changes

mergify bot added needs: work and removed needs: review labels Jul 1, 2020

jamesbeyond previously requested changes Jul 1, 2020

View reviewed changes

dustin-crossman added 2 commits July 1, 2020 10:38

dustin-crossman force-pushed the pr/sleep_test_fix branch from decf2eb to 0ae7516 Compare July 1, 2020 18:00

0xc0170 approved these changes Jul 2, 2020

View reviewed changes

0xc0170 added needs: review and removed needs: work labels Jul 2, 2020

mergify bot added needs: CI and removed needs: review labels Jul 2, 2020

mergify bot added ready for merge and removed needs: CI labels Jul 2, 2020

0xc0170 added the release-type: patch Indentifies a PR as containing just a patch label Jul 3, 2020

0xc0170 merged commit fe2fb48 into ARMmbed:master Jul 3, 2020

mergify bot removed the ready for merge label Jul 3, 2020

0xc0170 mentioned this pull request Jul 3, 2020

Revert "Fix kv parsing bug in greentea client." #13227

Merged

dustin-crossman mentioned this pull request Jul 6, 2020

Fix kv parsing bug in greentea client #13240

Closed

dustin-crossman mentioned this pull request Jul 14, 2020

nightly: CY8CPROTO_062_4343W target failed at sleep/deep sleep test #12434

Closed

adbridge added release-version: 6.2.0 Release-pending and removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Jul 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix hal-sleep/sleep_manager Tests on Cypress Targets #13210

Fix hal-sleep/sleep_manager Tests on Cypress Targets #13210

dustin-crossman commented Jun 30, 2020

ciarmcom commented Jun 30, 2020

0xc0170 commented Jul 1, 2020

0xc0170 left a comment

jamesbeyond Jul 1, 2020

dustin-crossman Jul 1, 2020

0xc0170 Jul 2, 2020

jamesbeyond Jul 2, 2020

dustin-crossman Jul 2, 2020

jamesbeyond Jul 3, 2020

dustin-crossman commented Jul 1, 2020

0xc0170 commented Jul 2, 2020

0xc0170 commented Jul 2, 2020

mbed-ci commented Jul 2, 2020

0xc0170 commented Jul 3, 2020

jamesbeyond commented Jul 3, 2020

0xc0170 commented Jul 3, 2020

Fix hal-sleep/sleep_manager Tests on Cypress Targets #13210

Fix hal-sleep/sleep_manager Tests on Cypress Targets #13210

Conversation

dustin-crossman commented Jun 30, 2020

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

ciarmcom commented Jun 30, 2020

0xc0170 commented Jul 1, 2020

0xc0170 left a comment

Choose a reason for hiding this comment

jamesbeyond Jul 1, 2020

Choose a reason for hiding this comment

dustin-crossman Jul 1, 2020

Choose a reason for hiding this comment

0xc0170 Jul 2, 2020

Choose a reason for hiding this comment

jamesbeyond Jul 2, 2020

Choose a reason for hiding this comment

dustin-crossman Jul 2, 2020

Choose a reason for hiding this comment

jamesbeyond Jul 3, 2020

Choose a reason for hiding this comment

dustin-crossman commented Jul 1, 2020

0xc0170 commented Jul 2, 2020

0xc0170 commented Jul 2, 2020

mbed-ci commented Jul 2, 2020

Test run: SUCCESS

0xc0170 commented Jul 3, 2020

jamesbeyond commented Jul 3, 2020

0xc0170 commented Jul 3, 2020