Add bare metal test for UART. #5273

mprse · 2017-10-09T06:58:01Z

Description

This is a proposition of UART bare metal test for Green Tea.

Requirements:

When vendors start porting a platform to mbed OS, they need to take big steps before they actually can start running tests. One of the first requirements to run greentea is functional UART (RawSerial), unfortunately there's nothing that would tell whether the implementation is correct.
Analize greentea to identify serial primitives that need to be tested to ensure the greentea will work correctly.
Create tests set:
    it shouldn't use greentea
    can be run on minimal (not complete) mbed OS port
    doesn't use other part of the system
    tests enough of UART to enable greentea
    doesnt need to be full unit/integration tests for UART

Commit provides 2 files:

test.py - host part of the test. This script builds mbed part of the test and performs serial communication test.
main.cpp - source file for mbed part of the test. This part tries to communicate with the host test using RawSerial class used by the Green Tea.

Example usage:

python test.py -t GCC_ARM -m NUCLEO_F070RB -b 9600 - run test using GCC_ARM toolchain, NUCLEO_F070RB board and test serial port with baud rate set to 9600 b/s.
python test.py -h - see detail usage information.

Status

HOLD - test location needs to be changed

Migrations

NO

mprse · 2017-10-09T07:02:38Z

@maciejbocianski @fkjagodzinski - Ready for review.

0xc0170 · 2017-10-09T08:23:08Z

TESTS/mbed_hal/uart_bare_metal_test/test_config.h

@@ -0,0 +1,3 @@
+/* This file has been automatically generated by the python script. */
+/* This file contains serial port baud rate. */
+#define BAUD_RATE 19200


We could use config for this? to create Config object, add this attribute there and generate config file that would be ignored by git?

In case, this is the best approach, this file possibly should be created by a test and being ignored in git? Thus not part of this patch?

This file is already generated by the script based on provided script parameter.
I will remove this file from commit.

0xc0170 · 2017-10-09T08:24:00Z

TESTS/mbed_hal/uart_bare_metal_test/main.cpp

+ * function returns FAILED status.
+ *
+ * */
+test_status test_uart_communication(RawSerial &serial, const char * msg)


doxygen comments, in separate header file?

I'm not sure if we need this for this test. This test must be executed manually and does not use grean tea. The hart of this test is the python script and it provides detailed description.

0xc0170 · 2017-10-09T12:40:23Z

retest uvisor

fkjagodzinski

Python script lacks portability -- I suppose this is not OK.

fkjagodzinski · 2017-10-09T13:48:44Z

TESTS/mbed_hal/uart_bare_metal_test/uart_test.py

+ call host test with appropriate --baudrate parameter.
+
+ Example usage:
+ uart_test.py -m K64F -m K64F -t GCC_ARM -b 9600


Typo: -m K64F -m K64F.

fkjagodzinski · 2017-10-09T13:49:19Z

TESTS/mbed_hal/uart_bare_metal_test/uart_test.py

+VERBOSE = False
+RETRY_LIMIT = 10
+
+# Listo of expected messages from the mbed device.


Typo: Listo.

fkjagodzinski · 2017-10-09T13:57:56Z

TESTS/mbed_hal/uart_bare_metal_test/uart_test.py

+ 'PASSED\0']
+
+def log_info(msg):
+ print('[INFO]: %s') % msg


I suggest using Python's logging module for logging.

Thanks for the tip, but for now I decided to stay with my log functions. After the update they are only simple wrappers to print function. This gives additional flexibility. I can have log functions to indicate failure, pass and received/transmitted data.

And how is this an advantage over the logging module?

This solution gives me everything what is needed to print out logs and allows to format log messages. Of course there is no problem to use logging module instead.

I'd recommend moving to logging as well. I know the tools don't currently support Python 3, but in the future we'll have to (Python 2 is only minimally being maintained). logging has Python 3 support out of the box, where as this current implementation does not.

Modified logging method as suggested.

fkjagodzinski · 2017-10-09T14:00:28Z

TESTS/mbed_hal/uart_bare_metal_test/uart_test.py

+ print('[FAIL]: %s') % msg
+ if(serial_port != None and serial_port.isOpen() == True):
+ serial_port.close()
+ sys.exit()


That is way too big side effect for a log function.

Removed additional actions performed by the log function.

fkjagodzinski · 2017-10-09T14:40:02Z

TESTS/mbed_hal/uart_bare_metal_test/uart_test.py

+ log_fail('Command can not be executed.')
+ sys.exit()
+
+ output = str()


Unused variable output.

fkjagodzinski · 2017-10-09T14:53:55Z

TESTS/mbed_hal/uart_bare_metal_test/uart_test.py

+ # When dumping output to file both \r and \n will be a new line
+ # To avoid this "extra new-line" we only use \n at the end
+ sys.stdout.write(line.rstrip() + '\n')
+ sys.stdout.flush()


Use Popen(cmd, stdout=sys.stdout) instead.

Fixed as suggested.

fkjagodzinski · 2017-10-09T15:23:34Z

TESTS/mbed_hal/uart_bare_metal_test/uart_test.py

+ if('ERROR' in line):
+ build_success = False
+ if('Image:' in line):
+ image_path = line[7:]


More stable API sould be used instead of parsing stdout. Use mbed test --compile --build-data foo and then parse foo JSON file:

build_data = {} with open('foo') as f: build_data = json.load(f) try: image_path = build_data['builds'][0]['bin'] except KeyError: image_path = '' # or raise an exception

Fixed as suggested.

fkjagodzinski · 2017-10-09T15:28:55Z

TESTS/mbed_hal/uart_bare_metal_test/uart_test.py

+ # for like COMnotanumber
+ pass
+
+ hComPort = win32.CreateFile(port, win32.GENERIC_READ | win32.GENERIC_WRITE, 0, None, win32.OPEN_EXISTING, win32.FILE_ATTRIBUTE_NORMAL | win32.FILE_FLAG_OVERLAPPED, 0)


The assumption of using a Windows machine should be documented.

Note has been added to the script description.

You should just not assume that you're on a windows machine! We have linux machines in CI.

I agree, but currently I don't have linux machine.

mbed-ci · 2017-10-10T13:47:06Z

Build : FAILURE

Build number : 58
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5273/

0xc0170 · 2017-10-12T10:39:41Z

@mprse jenkins CI fails, related to this patch, please check

mprse · 2017-10-12T12:03:55Z

@0xc0170

@mprse jenkins CI fails, related to this patch, please check

I'm not sure if there is a sens to check this tests on build machines. It needs to be executed manually.
Jenkins fails with the following error:

Error: #5: cannot open source input file "test_config.h": No such file or directory

test_config.h is a config file generated by python script (has been removed from this patch).

mprse · 2017-10-12T12:12:55Z

Modified function to reset the mbed board. Current implementation should be valid for Windows and Linux (not tested).

mbed-ci · 2017-10-12T12:33:27Z

Build : FAILURE

Build number : 150
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5273/

0xc0170 · 2017-10-12T14:00:02Z

test_config.h is a config file generated by python script (has been removed from this patch).

I see, than this test should not be automatically run, as it has this python dependency. is it required or how to make it CI friendly?

mprse · 2017-10-13T07:37:40Z

I see, than this test should not be automatically run, as it has this python dependency. is it required or how to make it CI friendly?

Honestly I'm not familiar with CI, maybe we should put it in some place where CI is not performed.

This test is for vendors to be able to verify if their serial HAL drivers are ready to use Grean Tea. To do this python script should be run with some parameters. This script builds the embed app, loads it on the board and run it. This embed app uses only RawSerial class and tries to communicate with PC (python script) by sending sample strings with different length and waiting for the responses. If all strings are successfully transmitted in both sides, then test is passed.

0xc0170 · 2017-10-13T07:53:56Z

Honestly I'm not familiar with CI, maybe we should put it in some place where CI is not performed.

cc @studavekar @theotherjimmy - how to handle this type of test?

studavekar · 2017-10-16T01:24:14Z

We can add in check, if and only if there is a label "needs: CI" then we start CI.

0xc0170 · 2017-10-19T15:13:41Z

This test is for vendors to be able to verify if their serial HAL drivers are ready to use Grean Tea. To do this python script should be run with some parameters. This script builds the embed app, loads it on the board and run it. This embed app uses only RawSerial class and tries to communicate with PC (python script) by sending sample strings with different length and waiting for the responses. If all strings are successfully transmitted in both sides, then test is passed.

@studavekar @bridadan How we can handle this one, via ignore or any other suggestions?

bridadan · 2017-10-19T16:34:16Z

TESTS/mbed_hal/uart_bare_metal_test/uart_test.py

+ 'PASSED\0']
+
+def log_info(msg):
+ print('[INFO]: %s') % msg


I'd recommend moving to logging as well. I know the tools don't currently support Python 3, but in the future we'll have to (Python 2 is only minimally being maintained). logging has Python 3 support out of the box, where as this current implementation does not.

bridadan · 2017-10-19T16:36:18Z

TESTS/mbed_hal/uart_bare_metal_test/uart_test.py

+ required = parser.add_argument_group('required arguments')
+ optional = parser.add_argument_group('optional arguments')
+ required.add_argument("-m", "--target", help="Target id")
+ optional.add_argument("-t", "--toolchanin", help="Compile toolchain. Example: ARM, GCC_ARM, IAR.", default='GCC_ARM')


Typo here, should be --toolchain

bridadan · 2017-10-19T16:42:57Z

@studavekar @bridadan How we can handle this one, via ignore or any other suggestions?

One option would be to create a new folder in mbed OS for these kinds of tests. This one would be included in a .mbedignore.

To actually build the test, you could force tools to ignore the .mbedignore by using the --source option:

/mbed-os
   - /bare_metal_tests
       - /.mbedignore (contents would be just "*" with no quotes)
       - /uart
             - /main.cpp
             - /test.py

Where test.py would call mbed-cli as:

mbed test --source <path to mbed-os root> --source <path to mbed-os root>/bare_metal_tests/uart

mprse · 2017-10-23T07:50:08Z

@bridadan I tried to use "mbed test --source" in current tree folder where test is located in:
/m-bed/TESTS/mbed_hal/uart_bare_metal_test/main.cpp

I have executed the following command:
mbed test --source . --source TESTS/mbed_hal/uart_bare_metal_test -m K64F -t GCC_ARM -c
The compilation process have failed. It looks like all test source files have been included into this build since I get multiple definition of `main' error messages.

Currently in python script I use mbed --compile to build test and I have my functions for flashing and resetting the board. As far as I understand these function will be redundant. Command
mbed test --source . --source TESTS/mbed_hal/uart_bare_metal_test -m K64F -t GCC_ARM -c
will flash and reset the board for me. Correct?

@0xc0170
Shell I modify this pull request and update the directory structure to by consistent with proposition provided by @bridadan?

theotherjimmy · 2017-10-23T15:10:46Z

@0xc0170 @bridadan @studavekar Could you review?

bridadan · 2017-10-23T22:32:52Z

The compilation process have failed. It looks like all test source files have been included into this build since I get multiple definition of `main' error messages.

Darn it, you're right. I forgot that that's how that particular script works.

I did some more digging and learned that I goofed when I originally wrote the file. It doesn't actually use the --source flag when it's searching for tests, instead it uses your current working directory or the -p flag, which I don't think anything really uses. This is a problem for this kind of scenario.

SO I patched it: bridadan@f8ed507

You'll also need to update your directory structure:

/mbed-os
- /.mbedignore (contents would be just "*" with no quotes)
- /bare_metal_tests
    - /TESTS
        - /bringup
            - /uart
                - /main.cpp
                - /test.py

And then the CLI command would look like:

cd mbed-os
mbed test -m K64F -t GCC_ARM --source . --source bare_metal_test

I have to say, I'm not really seeing that many advantages this has over just using Greentea/htrun. It basically invokes the build tools, copies the binaries, resets the target, and compare strings in the Python script. Which basically describes Greentea in a nutshell.

It looks like you're citing a engineering requirement @mprse, can you tell us where this is from?

mprse · 2017-10-24T10:47:15Z

@bridadan Thank you for clarifications.

The cited requirement can be found here:
https://jira.arm.com/browse/IOTMORF-1244

I have updated directory tree and with your patch to test.py I was able to run the mbed part of the test with:
mbed test -m K64F -t GCC_ARM --source . --source bare_metal_test command. As we expect this command successfully built the embed test, loaded the binary file on the device and reset the device. Unfortunately after this operation script sends synchronisation packets to the device and gets no response. This results in synchronisation error after a while. For me the best option would be to end the mbed test command after the reset of the board, but it looks like it is not possible. For now I have updated build command in python part of the test to the following form : mbed test -m K64F -t GCC_ARM --source . --source bare_metal_test --compile and left my functions to flash and reset the target.

mprse · 2017-11-30T10:58:57Z

Directory structure has been proposed by @bridadan and I thought that this is the final location. Maybe the valid location would be features directory?

adbridge · 2017-11-30T11:15:07Z

I agree with @0xc0170 I think a suite of tests that are geared purely for the porting guide would benefit from being in their own repo. Mbed-os clone is big enough already without cluttering with additional tests that are not directly related to the codebase. These tests are no more related to mbed-os itself than say an mbed-os-example is.

bulislaw · 2017-11-30T15:03:33Z

From what I rememember @bridadan and @theotherjimmy suggested to put it in TEST directory and add mbedignore so that it's ignored by mbed test command.

mprse · 2017-12-05T07:12:14Z

@0xc0170 @bulislaw @acabarbaye

It can't go into TESTS, not in tools..

From what I rememember @bridadan and @theotherjimmy suggested to put it in TEST directory and add mbedignore so that it's ignored by mbed test command.

Do we have some decision about the test location?
If TEST directory can be used it looks like it is the most suitable place.

bulislaw · 2017-12-05T10:39:15Z

@bridadan @theotherjimmy is there anything stoping us from having it in TEST/?

bridadan · 2017-12-05T15:05:36Z

@bulislaw

From what I rememember @bridadan and @theotherjimmy suggested to put it in TEST directory and add mbedignore so that it's ignored by mbed test command.

Pretty sure that should be fine!

theotherjimmy · 2017-12-05T15:25:57Z

@bulislaw That should be fine.

sg- · 2017-12-06T16:51:33Z

Why doesn't the test case use unity?

bridadan · 2017-12-06T16:54:30Z

@sg- Unity relies on utest for printing: https://github.com/ARMmbed/mbed-os/blob/master/features/frameworks/unity/unity/unity_config.h#L38

And utest relies on Greentea for printing:

mbed-os/features/frameworks/utest/source/unity_handler.cpp

Line 38 in 729ef15

greentea_serial->putc(chr);

And the whole point of this test is to not use Greentea.

sg- · 2017-12-06T18:32:45Z

And the whole point of this test is to not use Greentea.

So if void utest_safe_putc(int chr) was WEAK that would make the whole thing fit? I could imagine wanting to use SWO, semihosting or a DCC like system to put and get data from a physical transport.

bridadan · 2017-12-06T20:54:24Z

@sg- after looking at this a bit more, I'm now remembering that you can't use Unity outside of utest. This is due to the way Unity allows you to jump out of a utest test case immediately as soon as an assert fails (known as "Fail and bail": https://github.com/ARMmbed/mbed-os/blob/master/features/frameworks/unity/source/unity.c#L20).

Unity asserts only work in the context of a utest test case. So if you wanted to use Unity asserts in non-utest test cases, some further refactoring would need to take place.

sg- · 2017-12-06T22:30:38Z

In that case, is this essentially a greentea self-test?

bulislaw · 2017-12-07T10:12:33Z

I wouldn't say so as it doesn't test greentea, it validates that your serial driver is good enough to run further tests (which use greentea).

mprse · 2017-12-12T09:49:18Z

Do we have final decision about the localisation of this test?

0xc0170 · 2017-12-13T17:32:39Z

@bridadan @theotherjimmy is there anything stoping us from having it in TEST/?

This got voted, can we place it there

adbridge · 2017-12-29T14:23:08Z

@mprse Looks like this now needs a rebase ?
@sg- Do you have an opinion on whether the bare metal tests should actually live within mbed-os or have their own repo? Considering they are effectively just testing that parts of the hardware themselves function sufficiently to actually run mbed tests on them ?

This test is for Vendors to check if uart (RawSerial) drivers are ready to use green-tea. Example usage: python test.py -t GCC_ARM -m NUCLEO_F070RB -b 9600 - run test using GCC_ARM toolchain, NUCLEO_F070RB board and test serial port with baud rate set to 9600 b/s.

mprse · 2018-01-08T10:31:37Z

@mprse Looks like this now needs a rebase ?

Fixed conflicts - updated Travis script.

mprse · 2018-01-10T12:41:39Z

@0xc0170 @sg @bulislaw @bridadan @theotherjimmy @adbridge - can we make a final decision here about the test location?

adbridge · 2018-01-10T14:00:57Z

I still maintain these should go to their own repository. The logic behind this is quite clear, they are an alternative to Greentea to ascertain whether or not the drivers are in a suitable state to be able to work with Greentea. Greentea has it's own repository so why shouldn't this ?
Also having to add extra guards/mbed ignores etc to try and shoehorn something in which obviously isn't designed to fit somewhere in the first place, tells me it's the wrong option.

bridadan

I'm a bit out-of-touch on this, so feel free to take my opinion with a grain of salt. I have no problem with this here as long as its documented well. It also has the benefit that I don't have to find another repository to clone.

But I understand @adbridge's view and think its valid as well. In my view I think this just needs an executive decision on where this lives.

cmonr · 2018-01-11T17:21:03Z

Will need to be put into another repo.

0xc0170 reviewed Oct 9, 2017

View reviewed changes

0xc0170 added the needs: review label Oct 9, 2017

fkjagodzinski suggested changes Oct 9, 2017

View reviewed changes

mprse force-pushed the uart_test branch from 13dcebb to 99c874d Compare October 10, 2017 12:37

mprse force-pushed the uart_test branch from 99c874d to 2c8e34f Compare October 12, 2017 06:25

0xc0170 added needs: work and removed needs: review labels Oct 12, 2017

mprse force-pushed the uart_test branch from 2c8e34f to 47c890d Compare October 12, 2017 12:11

bridadan reviewed Oct 19, 2017

View reviewed changes

mprse force-pushed the uart_test branch from 47c890d to 9da0739 Compare October 20, 2017 07:33

theotherjimmy added needs: review and removed needs: work labels Oct 23, 2017

mprse force-pushed the uart_test branch from 9da0739 to 594425d Compare October 24, 2017 09:59

adbridge added needs: work and removed needs: review labels Dec 29, 2017

mprse force-pushed the uart_test branch from ade157c to 95adbc5 Compare January 8, 2018 10:29

Add uart bare metal test to Travis.

0c43ed0

mprse force-pushed the uart_test branch from 95adbc5 to 0c43ed0 Compare January 8, 2018 11:35

bridadan reviewed Jan 10, 2018

View reviewed changes

cmonr closed this Jan 11, 2018

cmonr removed the needs: work label Jan 11, 2018

mprse mentioned this pull request Jan 12, 2018

Add low-level test for uart. mprse/mbed-os-lowlevel-tests#1

Closed

Add bare metal test for UART. #5273

Add bare metal test for UART. #5273

Conversation

mprse commented Oct 9, 2017 • edited Loading

Description

Status

Migrations

mprse commented Oct 9, 2017

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

0xc0170 commented Oct 9, 2017

fkjagodzinski left a comment • 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbed-ci commented Oct 10, 2017

0xc0170 commented Oct 12, 2017

mprse commented Oct 12, 2017

mprse commented Oct 12, 2017

mbed-ci commented Oct 12, 2017

0xc0170 commented Oct 12, 2017

mprse commented Oct 13, 2017

0xc0170 commented Oct 13, 2017

studavekar commented Oct 16, 2017

0xc0170 commented Oct 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridadan commented Oct 19, 2017

mprse commented Oct 23, 2017

theotherjimmy commented Oct 23, 2017

bridadan commented Oct 23, 2017

mprse commented Oct 24, 2017

mprse commented Nov 30, 2017

adbridge commented Nov 30, 2017 • edited Loading

bulislaw commented Nov 30, 2017

mprse commented Dec 5, 2017

bulislaw commented Dec 5, 2017

bridadan commented Dec 5, 2017

theotherjimmy commented Dec 5, 2017

sg- commented Dec 6, 2017

bridadan commented Dec 6, 2017

sg- commented Dec 6, 2017 • edited Loading

bridadan commented Dec 6, 2017

sg- commented Dec 6, 2017

bulislaw commented Dec 7, 2017

mprse commented Dec 12, 2017

0xc0170 commented Dec 13, 2017

adbridge commented Dec 29, 2017

mprse commented Jan 8, 2018

mprse commented Jan 10, 2018

adbridge commented Jan 10, 2018 • edited Loading

bridadan left a comment

Choose a reason for hiding this comment

cmonr commented Jan 11, 2018

mprse commented Oct 9, 2017 •

edited

Loading

fkjagodzinski left a comment •

edited

Loading

adbridge commented Nov 30, 2017 •

edited

Loading

sg- commented Dec 6, 2017 •

edited

Loading

adbridge commented Jan 10, 2018 •

edited

Loading