Fix kv parsing bug in greentea client

[dustin-crossman]

Summary of changes

Fixes kv-parsing bug in greentea client.
This commit was previously part of #13210 but was merged then reverted because discussion was not finalized.

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)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Test results provided as part of #13210

---

Reviewers

@0xc0170
@jamesbeyond

---

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

Quoting @jamesbeyond last message on the old PR so we can continue on that discussion:

Can you show where in the documentation the kv format is expected to handle both a \n and \r\n ending? The only spot in those links I can find is here: https://github.com/ARMmbed/mbed-os-tools/tree/master/packages/mbed-host-tests#design-draft and that states that:

Transport layer consist of simple {{ KEY ; VALUE }} \n text messages sent by slave (DUT). Both key and value are strings with allowed character set limitations (to simplify parsing and protocol parser itself). Message ends with required by DUT K-V parser \n character.

I do see that there is more inconsistency on the GT C++ side: It appears the c++ client sends kv pairs with the \r\n: https://github.com/ARMmbed/mbed-os/blob/master/features/frameworks/greentea-client/source/greentea_test_env.cpp#L234
but (before this PR) read them without any ending at all.

[dustin-crossman]

It looks like there is more inconsistency on the python as well:
Python GT reads KV pairs without a line ending: https://github.com/ARMmbed/mbed-os-tools/blob/master/src/mbed_os_tools/test/host_tests_conn_proxy/conn_proxy.py#L33
But writes them with a \n line ending: https://github.com/ARMmbed/mbed-os-tools/blob/master/src/mbed_os_tools/test/host_tests_conn_proxy/conn_primitive.py#L39

[ciarmcom]

@dustin-crossman, thank you for your changes.
@jamesbeyond @0xc0170 @ARMmbed/mbed-os-maintainers please review.

[0xc0170]

It looks fine to me. Do you have a test case that we could reproduce this bug to show it was the real issue?

[dustin-crossman]

@0xc0170 I don't have a specific test case for this issue but you can see that if you run the sleep tests on a Cypress target they will still fail on the current master (338e5ea). Merging this commit back in will cause the tests to pass.

[0xc0170]

CI started, lets see how tests go

[jamesbeyond]

Hi, @dustin-crossman,

I can see there are some inconsistent here and there. but for the design principle of the GreenTea, Only the pattern {{KEY;VALUE}} is considered as valid. everything outside {{ }} should/will be ignored. That is the reason why we don't want to enforce \n part of the end token.
Further more make \n compulsory clear breaks some windows user case. that is why I need this PR to be changed.
I proposed a much simpler change here: #13247

[mbed-ci]

Test run: SUCCESS

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

[0xc0170]

I am closing this for now, lets review #13247