Add C API for Greentea client #4364

mazimkhan · 2017-05-22T13:08:03Z

Description

mbedtls unit testing framework is enhanced to execute on target. PR Mbed-TLS/mbedtls#930
Since the mbed-tls is a C library and its unit test framework is also written in C. In order to run it with Greentea we need C API for the Greentea client in mbed-os. This API adds extern "C" wrappers for C linkage of Greentea Client API needed by mbedtls unit test.

Status

READY

Migrations

If this PR changes any APIs or behaviors, give a short description of what API users should do when this PR is merged.

NO

Related PRs

PR in mbedtls Mbed-TLS/mbedtls#930

theotherjimmy · 2017-05-22T14:45:30Z

@mazimkhan Could you fill out the relevant sections of the template and remove the rest?

theotherjimmy · 2017-05-22T14:47:47Z

@mazimkhan Could you change the title to match the guidelines for a good commit message? In particular, could you update it to be in the imperative mood? https://chris.beams.io/posts/git-commit/ We use PR titles in the release notes.

mazimkhan · 2017-05-22T15:47:25Z

@theotherjimmy Thanks for pointing this out.

mazimkhan · 2017-05-22T17:41:19Z

@sg- @bridadan @adbridge @0xc0170 Please review and merge.

sg-

Why are new functions created rather than adding extern "C" to the existing greentea functions? Given the implementation is still in .cpp I seems that is what you meant todo?

bridadan · 2017-05-22T23:08:16Z

Agreed with @sg-, can the existing function for which you've added the c wrappers just be moved to a .c file? Seems like that should be non breaking change (could be missing something though). I'd rather avoid creating wrappers for functions.

adbridge · 2017-05-23T10:17:43Z

Inline with the above 2 comments . '_c' function names are pretty horrible ...

mazimkhan · 2017-05-23T11:03:14Z

@sg- @bridadan I understand what you mean. But since I had to create a new test_env_c.h for including in my C test code I thought of keeping everything completely separate. test_env.h has polymorphic functions named greentea_send_kv that prevent inclusion in C code.

But I have consolidate it together to use the same functions. Please review updated code.

pan- · 2017-05-23T13:42:59Z

features/frameworks/greentea-client/greentea-client/test_env.h

@@ -28,6 +28,9 @@
 #endif

 #include <stdio.h>
+extern "C" {


Why not put these functions into this header and exclude C++ parts:

// C declaration block #ifdef __cplusplus extern "C" { #endif void GREENTEA_SETUP(const int timeout, const char * host_test); void greentea_send_kv(const char * key, const char * val); int greentea_parse_kv(char * key, char * val, const int key_len, const int val_len); #ifdef __cplusplus } #endif // C++ declaration block #ifdef __cplusplus // put C++ functions here #endif

isn't it cleaner without lots of ifdefs?

Unfortunately there is trade of to make, I don't believe we will get any thing very clean out of this.

Splitting definitions into separate file does not reduce the number of ifdef in the code or if it does it is at the expense of user friendliness.

First of all consider that if the proposed solution is chosen then declaration in test_env_c.h shall be guarded with

#ifdef __cplusplus extern "C" { #endif // declaration #ifdef __cplusplus } #endif

Otherwise if ever a C++ file include test_env_c.h the functions declared will be seen as C++ symbols. Note that the inclusion guard in test_env.h can be removed.

Secondly, to allow inclusion of test_env.h within C code then C++ bits have to be guarded.

None of those guards are mandatory but they improve the end user experience.

OK. Actually that's the point of splitting into two files. The test_env.h is included only by C++ code hence does not need ifdef __cplusplus. Similarly test_enc_c.h is only included in the C code (as suggested by '_c') hence no need to put extern "C" there.

@sg- If a module expose a C and a C++ API what is the mbed way of exposing its APIs ?

This hasn't really come up before. There has always been clear separation between C and C++ code. Question really is why this isn't the case now for greentea? I'd guess this has to do with using RawSerial and Tickers in the test harness?? @mazimkhan are you planning to bind greentea to mbed OS drivers or a different runtime for put/get?

I would prefer a standard runtime of getc/putc. But I don't intend to change existing implementation here. I need Greentea API for the mbedtls C test code.
Are you suggesting C implementation of Greentea API. It seems reasonable since it has no C++.

As @pan- suggested, one header file and make it compatible for both languages. Thats the entry point for tests (test_env.h). C++ can use both provided API, C only C part of it.

Ok if that is the practice. But we are making this header common for C and C++.
For any odd reason we would want to switch to using .hpp or .hh extension for C++ headers than it would look odd.

I will update the code based on the suggestion.

pan- · 2017-05-23T13:45:01Z

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

@@ -532,7 +531,7 @@ enum Token {
 * \return Next character from the stream or EOF if stream has ended.
 *
 */
-static int _get_char() {
+extern "C" char greentea_getc() {


May it be in greentea_serial.cpp ?

Is there a reason to not return an int like the standard getc function ?

May it be in greentea_serial.cpp ?

May be but greentea_serial.cpp looks more like creation of the singleton greentea_serial object. Most of the API ('greentea_') implementation is in greentea_test_env.cpp. I just changed the name.

Is there a reason to not return an int like the standard getc function ?

No. reverted to int. Please review updated code.

May be but greentea_serial.cpp looks more like creation of the singleton greentea_serial object. Most of the API ('greentea_') implementation is in greentea_test_env.cpp. I just changed the name.

That is a good point and in some way greentea_getc is the counter part of greentea_send_kv which is declared in test_env.h.

pan- · 2017-05-23T13:48:24Z

features/frameworks/greentea-client/greentea-client/test_env_c.h

+void greentea_send_kv(const char * key, const char * val);
+int greentea_parse_kv(char * key, char * val,
+                        const int key_len, const int val_len);
+char greentea_getc();


This function belongs to greentea_serial.h module.
C and C++ parts can be isolated as shown above.

isn't it cleaner without lots of ifdefs?

sg- · 2017-06-03T05:05:12Z

@pan- approve the updates?

mazimkhan · 2017-06-05T09:33:56Z

@pan- @sg- Thanks. I am yet to make the updates. I will ping you once done.

mazimkhan · 2017-06-05T12:27:37Z

@pan- please review updated code.

pan-

LGTM

bridadan

Thanks for putting in the extra work @mazimkhan, this looks 1000x better! 👍

mazimkhan · 2017-06-05T15:11:37Z

Thanks @bridadan

0xc0170

It would be nice to have documentation for that public API but that is not the aim of this PR.

0xc0170 · 2017-06-05T15:23:56Z

@mazimkhan Can you rebase this PR (that should resolve jenkins CI) ?

mazimkhan · 2017-06-08T13:44:27Z

Can we retest below CI and merge this:
continuous-integration/jenkins/pr-head

0xc0170 · 2017-06-08T13:53:07Z

Jenkins CI restarted

bridadan · 2017-06-08T14:07:18Z

/morph test

mbed-bot · 2017-06-08T18:36:31Z

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 502

All builds and test passed!

tommikas · 2017-06-09T06:42:15Z

jenkins/pr-head should pass if you rebase.

The failing build is currently disabled in master because the build doesn't work with it.

mazimkhan · 2017-06-13T14:27:54Z

All tests pass and approvals received. Please merge it.

bridadan · 2017-06-13T15:46:09Z

/morph test

mbed-bot · 2017-06-13T19:39:52Z

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 544

All builds and test passed!

bridadan · 2017-06-13T19:45:55Z

@theotherjimmy LGTM

mazimkhan changed the title ~~Mbed tls test~~ Add C API for Greentea client May 22, 2017

mazimkhan mentioned this pull request May 22, 2017

Enable on-target execution of mbed TLS unit tests Mbed-TLS/mbedtls#930

Merged

sg- reviewed May 22, 2017

View reviewed changes

sg- added the needs: review label May 22, 2017

pan- requested changes May 23, 2017

View reviewed changes

pan- approved these changes Jun 5, 2017

View reviewed changes

bridadan approved these changes Jun 5, 2017

View reviewed changes

0xc0170 approved these changes Jun 5, 2017

View reviewed changes

0xc0170 added needs: CI and removed needs: review labels Jun 8, 2017

mazimkhan force-pushed the mbed-tls-test branch from a600e13 to a7e0e26 Compare June 9, 2017 15:42

Mohammad Azim Khan and others added 2 commits June 12, 2017 13:23

C API for greentea client

bd5820f

Add getc in C API

0edf082

Azim Khan added 3 commits June 12, 2017 13:46

Use existing functions for Greentea client C API

088d151

Revert greentea_getc() to return 'int'

82d948d

Merge greentea-client/test_env_c.h into greentea-client/test_env.h

b3cbb07

mazimkhan force-pushed the mbed-tls-test branch from a7e0e26 to b3cbb07 Compare June 12, 2017 13:03

Remove multiple definition of GREENTEA_SETUP added due to rebase

0b92919

theotherjimmy added ready for merge and removed needs: CI labels Jun 13, 2017

theotherjimmy merged commit 1ffbdfc into ARMmbed:master Jun 19, 2017

theotherjimmy added release-version: 5.5.1 and removed ready for merge labels Jun 19, 2017

Add C API for Greentea client #4364

Add C API for Greentea client #4364

Uh oh!

Conversation

mazimkhan commented May 22, 2017 • edited by theotherjimmy Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Status

Migrations

Related PRs

Uh oh!

theotherjimmy commented May 22, 2017

Uh oh!

theotherjimmy commented May 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mazimkhan commented May 22, 2017

Uh oh!

mazimkhan commented May 22, 2017

Uh oh!

sg- left a comment

Choose a reason for hiding this comment

Uh oh!

bridadan commented May 22, 2017

Uh oh!

adbridge commented May 23, 2017

Uh oh!

mazimkhan commented May 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pan- May 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mazimkhan May 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mazimkhan May 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sg- commented Jun 3, 2017

Uh oh!

mazimkhan commented Jun 5, 2017

Uh oh!

mazimkhan commented Jun 5, 2017

Uh oh!

pan- left a comment

Choose a reason for hiding this comment

Uh oh!

bridadan left a comment

Choose a reason for hiding this comment

Uh oh!

mazimkhan commented Jun 5, 2017

Uh oh!

0xc0170 left a comment

Choose a reason for hiding this comment

Uh oh!

mazimkhan commented May 22, 2017 •

edited by theotherjimmy

Loading

theotherjimmy commented May 22, 2017 •

edited

Loading

pan- May 23, 2017 •

edited

Loading

mazimkhan May 24, 2017 •

edited

Loading

mazimkhan May 23, 2017 •

edited

Loading