Skip to content
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.

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

Modify serial nc tests init part #1781

Merged
merged 2 commits into from
Jun 7, 2016
Merged

Conversation

LMESTM
Copy link
Contributor

@LMESTM LMESTM commented May 25, 2016

During initialization phase of the 2 NC serial tests,
there is a character being sent from host to target,
based on MBED_HOSTTEST_START. The problem is that
the target firmware then creates a "new" serial on the
same serial interface. the new or should, or at least may,
induce a reset of the IP, so that the character sent by host
would be lost. So we're moving the new earlier.

the real testing part happens later, to check the next
character is not received or sent.

@bridadan
Copy link
Contributor

@mbed-bot: TEST

HOST_OSES=ALL
BUILD_TOOLCHAINS=ALL
TARGETS=ALL

@mbed-bot
Copy link

[Build ${MBED_BUILD_ID}]
FAILURE: Something went wrong when building and testing.

@bridadan
Copy link
Contributor

@mbed-bot: TEST

HOST_OSES=ALL
BUILD_TOOLCHAINS=ALL
TARGETS=ALL

@mbed-bot
Copy link

[Build 403]
FAILURE: Something went wrong when building and testing.

@bridadan
Copy link
Contributor

Based on the test results (even though they say failed, those are older issues), this doesn't look it causes regressions in other platforms.

LGTM

@adustm
Copy link
Member

adustm commented May 26, 2016

Hello @bridadan ,
I just noticed that with this PR, the MBED_37 is no more testing getc while TX is at NC state.
Is it a problem or is it acceptable ?
Kind regards
Armelle

@bridadan
Copy link
Contributor

Hi @adustm, I missed that the first time, thanks for pointing it out!

That change is actually fine because that is not the behavior being tested.

@LMESTM
Copy link
Contributor Author

LMESTM commented May 26, 2016

Hi
So do you need me to update the test with more case (like Serial(NC, USBRX); )?
Laurent

@LMESTM
Copy link
Contributor Author

LMESTM commented May 26, 2016

I have shared a proposal here but I have not proposed it for pull request yet ... let me know if you think useful or not ... depending on what you say, this would require a wider testing (so far I tested on NUCLEO_F303RE and NUCLEO_F446RE)

@bridadan
Copy link
Contributor

I see what you're doing, though I'm not sure that I immediately see the benefit. The E character kind of acted like a sync before.

Can I ask what motivated this change? Are you seeing any improvements on your end?

In the meantime I'll run a bigger test to see what changes it has.

@bridadan
Copy link
Contributor

@mbed-bot: TEST

HOST_OSES=ALL
BUILD_TOOLCHAINS=ALL
TARGETS=ALL

@mbed-bot
Copy link

[Build 407]
FAILURE: Something went wrong when building and testing.

@LMESTM
Copy link
Contributor Author

LMESTM commented May 27, 2016

@bridadan Sorry if not clear.

I am proposing to add again a "getc while TX is at NC state" as suggested by adustm.
But to do this, I wanted to have a proper sync, which _ I think _ was not the case before.

Indeed, up to now target was calling MBED_HOSTTEST_START and just after that it was reconfiguring the serial link to "tx not connected + rx" - this was a raise condition to me because we're not sure whether the character from HOST would be sent before or after the new serial instance on TARGET will be created. Therefore, the character might be missed.

Maybe the thing is that I am not clear of the complete path on how SerialNCRXTest wlll be called. Maybe a simple "time.sleep(0.5)" on HOST would be enough to ensure we leave enough time to target to delete serial instance and create new one ...

@bridadan
Copy link
Contributor

Ah I see what you're saying, thanks for clarifying!

It looks like this change has caused a regression in the NUCLEO_L053R8, I'll need to do a little bit more testing. I haven't seen any other platforms have regressions from this though.

@LMESTM
Copy link
Contributor Author

LMESTM commented May 30, 2016

@bridadan What is the regression that you see on NUCLEO_L053R8 ?

@LMESTM
Copy link
Contributor Author

LMESTM commented May 31, 2016

@bridadan : How have you noticed a L053R8 regression ? when I try to follow 'details' link above I get an ERR_CONNECTION_TIMED_OUT

As far as I know the test result is mainly unstable so far ... so the possible errors you may see from time to time on NUCLEO boards shall anyway be fixed by another pull request:
#1801

c = pc->getc();
// This should be false/not get here
if (c == 'U') {
pc->printf("RX OK - Unexpected\r\n");
Copy link
Contributor

Choose a reason for hiding this comment

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

please fix the formatting. line 32, line 24 and 20 (tabs?) .

@LMESTM
Copy link
Contributor Author

LMESTM commented May 31, 2016

@0xc0170 I hopefully fixed the tabs issue in this new version (vim' expandtab option is definitely better than my hands made alignments )
Also Brian directly answered your questions - nothing to add from my side.

During initialization phase of the 2 NC serial tests,
there is a character being sent from host to target,
based on MBED_HOSTTEST_START. The problem is that
the target firmware then creates a "new" serial on the
same serial interface. the new or should, or at least may,
induce a reset of the IP, so that the character sent by host
would be lost. So we're moving the new earlier.

the real testing part happens later, to check the next
character is not received or sent.
We're ensuring target and host start-up sync here in 2 ways:
1) adding a delay on host side to make sure the serial
initialization can happen before sending a character is sent to target
2) in case of serial_nc_rx_auto.py test, we're sending a
first character S which will trigger the move from rx+tx
to NC+rx.

This should  avoid any crossing case due to HSOT being faster than target or vice-versa
@LMESTM
Copy link
Contributor Author

LMESTM commented Jun 3, 2016

@0xc0170
Branch just rebased,
Questions were answered I think - anything preventing a merge ?

@0xc0170 0xc0170 merged commit 4afc14d into ARMmbed:master Jun 7, 2016
@LMESTM LMESTM deleted the fix_MBED_37_Init branch July 15, 2016 11:40
artokin pushed a commit to artokin/mbed-os that referenced this pull request Aug 13, 2018
…..7963594

7963594 Merge branch 'release_internal' into release_external
9e31d11 Update apache license to config-files (ARMmbed#1781)
b8f840c Merge branch 'release_internal' into release_external
9495d94 Rename cfg-files to h-files (ARMmbed#1780)

git-subtree-dir: features/nanostack/sal-stack-nanostack
git-subtree-split: 7963594
artokin pushed a commit to artokin/mbed-os that referenced this pull request Aug 15, 2018
…..3abd82e

3abd82e Merge branch 'release_internal_v8.1.1' into release_external_v8.1.1
f2ca069 Update apache license to config-files (ARMmbed#1781)
b2179ee Rename cfg-files to h-files (ARMmbed#1780)

git-subtree-dir: features/nanostack/sal-stack-nanostack
git-subtree-split: 3abd82e07a733fbb84e786f138541b5a0ef50c26
cmonr pushed a commit that referenced this pull request Aug 24, 2018
…..3abd82e

3abd82e Merge branch 'release_internal_v8.1.1' into release_external_v8.1.1
f2ca069 Update apache license to config-files (#1781)
b2179ee Rename cfg-files to h-files (#1780)

git-subtree-dir: features/nanostack/sal-stack-nanostack
git-subtree-split: 3abd82e07a733fbb84e786f138541b5a0ef50c26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants