Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jul 3, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c70baa9
Fix kv parsing bug in greentea client.
dustin-crossman Feb 21, 2020
0ae7516
Fix serial wait bug in sleep test.
dustin-crossman Feb 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions TESTS/mbed_hal/sleep/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,17 +61,17 @@ void sleep_usticker_test()

const ticker_irq_handler_type us_ticker_irq_handler_org = set_us_ticker_irq_handler(us_ticker_isr);

/* Give some time Green Tea to finish UART transmission before entering
* sleep mode.
*/
busy_wait_ms(SERIAL_FLUSH_TIME_MS);

/* Test only sleep functionality. */
sleep_manager_lock_deep_sleep();
TEST_ASSERT_FALSE_MESSAGE(sleep_manager_can_deep_sleep(), "deep sleep should be locked");

/* Testing wake-up time 10 us. */
for (timestamp_t i = 100; i < 1000; i += 100) {
/* Give some time Green Tea to finish UART transmission before entering
* sleep mode.
*/
busy_wait_ms(SERIAL_FLUSH_TIME_MS);

/* note: us_ticker_read() operates on ticks. */
const timestamp_t start_timestamp = us_ticker_read();
const timestamp_t next_match_timestamp = overflow_protect(start_timestamp + us_to_ticks(i, ticker_freq),
Expand Down Expand Up @@ -108,15 +108,15 @@ void deepsleep_lpticker_test()

const ticker_irq_handler_type lp_ticker_irq_handler_org = set_lp_ticker_irq_handler(lp_ticker_isr);

/* Give some time Green Tea to finish UART transmission before entering
* deep-sleep mode.
*/
busy_wait_ms(SERIAL_FLUSH_TIME_MS);

TEST_ASSERT_TRUE_MESSAGE(sleep_manager_can_deep_sleep(), "deep sleep should not be locked");

/* Testing wake-up time 10 ms. */
for (timestamp_t i = 20000; i < 200000; i += 20000) {
/* Give some time Green Tea to finish UART transmission before entering
* deep-sleep mode.
*/
busy_wait_ms(SERIAL_FLUSH_TIME_MS);

/* note: lp_ticker_read() operates on ticks. */
const timestamp_t start_timestamp = lp_ticker_read();
const timestamp_t next_match_timestamp = overflow_protect(start_timestamp + us_to_ticks(i, ticker_freq), ticker_width);
Expand Down
27 changes: 15 additions & 12 deletions features/frameworks/greentea-client/source/greentea_test_env.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -523,7 +523,7 @@ static int CurTok = 0;
*
* tok_eof ::= EOF (end of file)
* tok_open ::= "{{"
* tok_close ::= "}}"
* tok_close ::= "}}\n"
Copy link
Contributor

@jamesbeyond jamesbeyond Jul 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@dustin-crossman dustin-crossman Jul 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@0xc0170 0xc0170 Jul 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@jamesbeyond jamesbeyond Jul 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@dustin-crossman dustin-crossman Jul 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@jamesbeyond jamesbeyond Jul 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* tok_semicolon ::= ";"
* tok_string ::= [a-zA-Z0-9_-!@#$%^&*()]+ // See isstring() function
*
Expand Down Expand Up @@ -597,7 +597,7 @@ extern "C" int greentea_parse_kv(char *out_key,

case tok_open:
if (HandleKV(out_key, out_value, out_key_size, out_value_size)) {
// We've found {{ KEY ; VALUE }} expression
// We've found {{ KEY ; VALUE }}\n expression
return 1;
}
break;
Expand Down Expand Up @@ -684,7 +684,7 @@ static int isstring(int c) {
*
* <TOK_EOF> ::= EOF (end of file)
* <TOK_OPEN> ::= "{{"
* <TOK_CLOSE> ::= "}}"
* <TOK_CLOSE> ::= "}}\n"
* <TOK_SEMICOLON> ::= ";"
* <TOK_STRING> ::= [a-zA-Z0-9_-!@#$%^&*()]+ // See isstring() function *
*
Expand Down Expand Up @@ -737,13 +737,16 @@ static int gettok(char *out_str, const int str_size) {
}

// close ::= '}'
if (LastChar == '}') {
LastChar = greentea_getc();
if (LastChar == '}') {
LastChar = '!';
return tok_close;
}
}
if (LastChar == '}') {
LastChar = greentea_getc();
if (LastChar == '}') {
LastChar = greentea_getc();
if (LastChar == '\n') {
LastChar = '!';
return tok_close;
}
}
}

if (LastChar == EOF)
return tok_eof;
Expand All @@ -762,8 +765,8 @@ static int gettok(char *out_str, const int str_size) {
* <MESSAGE>: <TOK_OPEN> <TOK_STRING> <TOK_SEMICOLON> <TOK_STRING> <TOK_CLOSE>
*
* Examples:
* message: "{{__timeout; 1000}}"
* "{{__sync; 12345678-1234-5678-1234-567812345678}}"
* message: "{{__timeout; 1000}}\n"
* "{{__sync; 12345678-1234-5678-1234-567812345678}}\n"
*
* \param out_key Output buffer to store key string value
* \param out_value Output buffer to store value string value
Expand Down